Simplifying Go Code

Question

I have two functions as shown below which look similar, but using different functions to query the db. Since Go doesn't encourage overloaading methods, is the redundancy acceptable? Or should I refactor them into one function? All comments are welcomed.

var getCustomers = func() ([]customer, error) {
    return nil, nil
}

var getCustomerById = func(int64) (*customer, error) {
    return nil, nil
}

func listCustomer(w http.ResponseWriter, r *http.Request) *appError {
    cus, err := getCustomers()
    if err != nil {
        return &appError{err, "No customer found", 404}
    }

    res, err := json.Marshal(cus)

    if err != nil {
        return &appError{err, "Can't display record", 500}
    }

    fmt.Fprint(w, string(res))
    return nil
}

func viewCustomer(w http.ResponseWriter, r *http.Request, id int64) *appError {
    cus, err := getCustomerByID(id)
    if err != nil {
        return &appError{err, "No customer found", 404}
    }

    res, err := json.Marshal(cus)

    if err != nil {
        return &appError{err, "Can't display record", 500}
    }

    fmt.Fprint(w, string(res))
    return nil
}

Suggestion to use -1 to list all customer, but I'm not sure if this is the best it can be:

func viewCustomer(w http.ResponseWriter, r *http.Request, id int64) *appError {
    var c *customer
    var clist []customer
    var err error

    if id < 0 {
        clist, err = getCustomers()
        res, err := json.Marshal(clist)

        if err != nil {
            return &appError{err, "Can't display record", 500}
        }

        fmt.Fprint(w, string(res))
        return nil
    } else {
        c, err = getCustomerById(id)
        res, err := json.Marshal(c)

        if err != nil {
            return &appError{err, "Can't display record", 500}
        }

        fmt.Fprint(w, string(res))
        return nil
    }
}

Answer 1

Since Go doesn't encourage overloaading methods, is the redundancy acceptable? Or should I refactor them into one function?

The answer to your question depends heavily on the real code and your task. If listing one customer is very different from listing several customers (that is, you need different information and have different presentation logic), I'd say that duplication here is not that bad, since the difference may grow larger in the future, so a DRY solution could turn into an if and switch mess quickly. (I've had a similar experience on a non-Go project and since then I think that DRY is good but you should not be fanatic about it.)

On the other hand, if you're making, say a JSON API, you could make it more DRY. Define your getCustomers function like this:

func customers(ids ...int64) ([]customer, error) { /* ... */ }

This way you can:

• call it like customers() and get all customers;
• call customers(42) and get a customer whose ID is 42;
• call customers(someIDs...) and get multiple customers by their IDs.

All can be done in one handler in a straightforward way.

All comments are welcomed.

Two nitpicks on your code:

• Getter methods in Go are idiomatically named foo and not getFoo, so I used customers in my example. You would most probably call it as DB.Customers(ids...), so it looks a lot like a getter method.

• What's up with var foo = func() notation? Unless there is a reason for that (like using these functions as closures), I'd suggest sticking with the func foo() notation, since it's more idiomatic and generally easier to grep. EDIT: as OP pointed out in the comments, another case for var foo = func() notation is of course functions that can be faked in testing, as shown in Andrew Gerrand's Testing Techniques talk.

I hope that helps.