Image Cropping and Resizing

Question

I am building a Go based image processing worker for my web application. I am trying to be idiomatic, but I am new to Go. Any feedback, is appreciated. This program uses bimg to generate 4 square images based on top, left, width, height, which come from a client side cropping library (croppie.js)

package main

import (
    "log"
    "os"
    "fmt"
    "strings"
    "gopkg.in/h2non/bimg.v1"
)

var sizes = [3]int{128, 256, 512}

func resizeImage(img []byte, width int, height int, name string) (err error){
    options := bimg.Options{
        Width: width,
        Height: height,
    }

    var thumb []byte

    thumb, err = bimg.Resize(img, options)
    if err != nil {
        log.Println(os.Stderr, err)
        return err
    }
    err = bimg.Write(name, thumb)
    return
}

func processImage(top, left, width, height int, filename string) (err error){
    var buffer, original []byte
    var image *bimg.Image
    var fileBaseName = strings.Split(filename, ".")[0]

    buffer, err = bimg.Read(filename)
    if err != nil {
        log.Println(os.Stderr, err)
    }

    image = bimg.NewImage(buffer)

    original, err = image.Extract(left, top, width, height)
    if err != nil {
        return err
    }

    err = bimg.Write(fileBaseName + "_original.jpg", original)
    if err != nil {
        return err
    }

    for i := 0; i < len(sizes); i++ {
        size := sizes[i]
        err = resizeImage(original, size, size, fmt.Sprintf(fileBaseName + "_%d.jpg", size))
        if(err != nil){
            log.Println(os.Stderr, err)
            return err
        }
    }
    return
}

func main() {
    err := processImage(50,50,400,400, "test2.jpeg")
    if(err != nil){
        log.Println(os.Stderr, err)
    }
}

Answer 1

All in all this code is decent for a beginner attempt at what is actually quite a sophisticated operation. Importing multiple libraries, logging, buffers, etc. Good job on just getting the job done.

There are a number of improvements though that you should consider. There are easier ones, and more complicated ones.

Function names

processImage is not a great name for a function. You may as well call it doStuff.... ;-) Choosing a name that represents the functions activity is a good way to confirm the function is doing just one thing (which is a goal of the SRP - single responsibility principle). A name like extractCropResizes would be closer, but that shows that the function is doing too many things, it's cropping, and resizing things. Consider splitting it in two, and calling both parts from the parent:

func extractCropResizes(....) {
    crop, err := cropImage(....)
    if err != nil {
        return err
    }
    extractResizes(crop, ...)
}

Globals

As for the actual functionality, it irks me that sizes is a global constant. I feel these resizes should be passed in as a variable to the looping resizing function. Globals tend to become problems when things need maintenance.

Error handling

Your error handling is incomplete, and inconsistent.

Your function returns an optional error, which is fine, but you're not trapping all the conditions. Consider this code:

buffer, err = bimg.Read(filename)
if err != nil {
    log.Println(os.Stderr, err)
}

If there is an error in there, you log the error, and then immediately will panic when the next line runs: image = bimg.NewImage(buffer) because buffer will be nil. Hmmm... I looked at the code, and no, the panic won't happen in the next line, but in the following one when the image is manipulated: Extract code.

Now, the consistency is also a problem. You declare the error return value to be called err in the function declaration: func processImage(top, left, width, height int, filename string) (err error)

If you use a named return value, then don't have return values in the return statement.

Also, no need to have a return as the last statement in the block.

Logging some errors to the console, and not others, is also inconsistent.

StdErr

Use the standard log functions to set the output destination of log messages log.SetOutput(os.StdErr) https://golang.org/pkg/log/#SetOutput and then don't use the option in each call.

Answer 2

Use range

The loop over sizes doesn't use the loop index, so it can be written more idiomatically using range:

for _, size := range sizes {
    // ...
}

Variable declarations

At many places you declare variables at the top of the function. This is neither necessary not recommended. It's best to declare variables right before you need them, not sooner.

Related to this, most functions use a named return value. This can be useful sometimes. But in the posted code, most of the time you return a value explicitly, and only use a parameterless return at the end of functions, so you might as well drop the naming and return nil where you need it.

Lastly, in resizeImage signature, you could shorten width int, height int like you already did in other functions.

Logging

I don't understand what os.Stderr is doing in the calls to log.Println. This function will write to standard error the parameters passed to fmt.Sprintf. If you want to log to standard error, you can replace log.Println(os.Stderr, err) with simply log.Println(err). The os.Stderr is not doing anything useful there, I suspect it was a symbol mistake.

Formatting

The code doesn't follow the standard at a few places. Run go fmt to fix it.