10

I'm trying to refactor the following node.js code.

Each case generates a thumbnail, chaining a different set of GraphicMagic transformation to an image.

switch(style.name) {
    case 'original':
        gm(response.Body)
            .setFormat('jpg').autoOrient().resize(style.w, style.h, style.option)
            .toBuffer(function(err, buffer) { if (err) { next(err); } else { next(null, buffer); } });
        break;
    case 'large':
        gm(response.Body)
            .setFormat('jpg').autoOrient().resize(style.w, style.h, style.option)
            .quality(style.quality)
            .strip().interlace('Plane')
            .toBuffer(function(err, buffer) { if (err) { next(err); } else { next(null, buffer); } });
        break;
    case 'medium':
        gm(response.Body)
            .setFormat('jpg').autoOrient().resize(style.w, style.h, style.option)
            .crop(style.w, style.h, style.crop.x_offset, style.crop.y_offset)
            .repage('+')
            .strip().interlace('Plane')
            .toBuffer(function(err, buffer) { if (err) { next(err); } else { next(null, buffer); } });
        break;
    case 'small':
        gm(response.Body)
            .setFormat('jpg').autoOrient().resize(style.w, style.h, style.option)
            .crop(style.w, style.h, style.crop.x_offset, style.crop.y_offset).repage('+')
            .quality(style.quality)
            .strip().interlace('Plane')
            .toBuffer(function(err, buffer) { if (err) { next(err); } else { next(null, buffer); } });
        break;
}

However, all cases share a number of transformations at the beginning and the end of the chaining, so there's room for refactoring. I tried refactoring with the following approach, but the code seems incorrect:

gm(response.Body)
.setFormat('jpg').autoOrient().resize(style.w, style.h, style.option, function(err, response) {
    if (style.name === 'original'){
        return response;
    } else if (style.name === 'large'){
        return response.quality(style.quality)
    } else if (style.name === 'medium'){
        return response.crop(style.w, style.h, style.crop.x_offset, style.crop.y_offset).repage('+')
    } else if (style.name === 'small'){
        return response.crop(style.w, style.h, style.crop.x_offset, style.crop.y_offset).repage('+').quality(style.quality)
    }
}).(function(response) {
    return (stryle.name !== 'original') ? response.strip().interlace('Plane') : return response;
}).(function(argument) {
    return response.toBuffer(function(err, buffer) { if (err) { next(err); } else { next(null, buffer); } });
});
Sbbs
  • 1,610
  • 3
  • 22
  • 34

2 Answers2

11

I wouldn't go for a switch at all.

There is no reason to use chaining at all here. Just do

// if (!/^(original|large|medium|small)$/.test(style.name)) throw new Error(…);
var x = gm(response.Body)
         .setFormat('jpg')
         .autoOrient()
         .resize(style.w, style.h, style.option);
if (style.name == "medium" || style.name == "small")
    x = x.crop(style.w, style.h, style.crop.x_offset, style.crop.y_offset)
         .repage('+');
if (style.name == "large" || style.name == "small")
    x = x.quality(style.quality);
if (style.name == "large" || style.name == "medium" || style.name == "small")
// possibly better than if (style.name != "original")
    x = x.strip()
         .interlace('Plane');
x.toBuffer(next);

But if you're having a large set of options so that it gets unreadable, better factor out each transformation in a function:

function resizedJpg(x) {
    return x.setFormat('jpg').autoOrient().resize(style.w, style.h, style.option);
}
function cropped(x) {
    return x.crop(style.w, style.h, style.crop.x_offset, style.crop.y_offset).repage('+');
}
function withQuality(x) {
    return x.quality(style.quality);
}
function stripped(x) {
    return x.strip().interlace('Plane');
}

And then apply them separately:

({
    original: [resizedJpg],
    large:    [resizedJpg,          withQuality, stripped],
    medium:   [resizedJpg, cropped,              stripped],
    small:    [resizedJpg, cropped, withQuality, stripped]
}[style.name]).reduce(function(x, trans) {
    return trans(x);
}, gm(response.Body)).toBuffer(next);
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • Thank you for the great answer! With the first approach, since we are not chaining all methods at once and use an intermediate variable x, will it run gm transformations separately (once before the if statements and twice after), or will it do them all at once when we call `toBuffer()`? – Sbbs Nov 24 '15 at 13:44
  • My concern is that calling transformations multiple times will also run gm multiple times on an image and is therefore computationally more expensive than chaining all transformations at once and running gm only once on an image. – Sbbs Nov 24 '15 at 13:58
  • 1
    @SBBS: No, `gm` cannot distinguish whether it is assigned to a variable or not, as long as the chain is executed synchronously. With this kind of builder pattern I'm pretty sure the work only happens when you call `.toBuffer()`. If it's well-written, you could call `.toBuffer()` twice on the same time and it will run exactly two times. – Bergi Nov 24 '15 at 18:07
5

I wrote small js helper for such usecases: https://github.com/sms-system/conditional-chain

With the help of this tool, you can chain methods optional, based on condition. Code will be more controlled and readable.

cond(gm(response.Body)
  .setFormat('jpg')
  .autoOrient()
  .resize(style.w, style.h, style.option))

.if(style.name == 'medium' || style.name == 'small', gm => gm
  .crop(style.w, style.h, style.crop.x_offset, style.crop.y_offset)
  .repage('+'))

.if(style.name == 'large' || style.name == 'small', gm => gm
  .quality(style.quality))

.if(style.name != 'original', gm => gm
  .strip()
  .interlace('Plane'))

.end().toBuffer(next)
Serge Smirnov
  • 51
  • 2
  • 3