1
\$\begingroup\$

My first ever attempt at Go. Writing a small app. It takes a property file and uses it to populate a Config object. After adding the first property of the Config object (dateformat) I realised that the code was looking horrible and would only get worse as I added more properties.

Please help me simplify, how should this be done? Why is my go-code always looking this horrible? :)

// Config holds the properties and values used for configuration
type Config struct {
    dateFormat string
}

// DefaultConfig is the set of configuration values used if none other is given.
var DefaultConfig = Config{"2006-01-02"}

// FromFile returns the configuration to use for did
func FromFile(path string) (c Config, e error) {
    c = DefaultConfig
    if lines, err := File2lines(path); err == nil {
        for _, line := range lines {
            prop, val, propErr := propertyOf(line)
            if propErr == nil && prop == "dateformat" && ValidDateFormat(val) {
                format, formatErr := ToGoDateFormat(val)
                if formatErr == nil {
                    c.dateFormat = format
                }
            } else if propErr != nil {
                e = propErr
            }
        }
    } else if err != nil {
        e = err
    }
    return
}

func propertyOf(str string) (key, val string, err error) {
    split := strings.Split(strings.TrimSpace(strings.TrimRight(str, "\n")), "=")
    if len(split) == 2 {
        return split[0], split[1], nil
    }
    return "", "", fmt.Errorf(fmt.Sprintf("String '%s' cannot be read as property.", str))
}

The code expect a file which has the following property:

dateformat=yyyymmdd
  • ValidDateFormat just checks if the given value is valid.
  • ToGoDateFormat converts any given format to valid golang date format
  • File2lines reads a file and returns its lines
\$\endgroup\$
  • \$\begingroup\$ Just odd that you decide to choose a file for config. The common approach in golang is to use environment variables and/or flags, not a Java-style properties file \$\endgroup\$ – Elias Van Ootegem Sep 18 '18 at 10:48
0
\$\begingroup\$

OK, there's a couple of comments I can make WRT code style looking at what you have now. Things that really stand out, for example, are lines like this:

return "", "", fmt.Errorf(fmt.Sprintf("String '%s' cannot be read as property.", str))

The function in the fmt package is called Errorf, where the f at the end stands for format. Why, then bother with the fmt.Sprintf call, just to get a string? fmt.Errorf does the same job as fmt.Sprintf, only it returns an error:

return "", "", fmt.Errorf("string '%s' can't be read as property", str)

Golang coding standards also states that error strings should start with a lower-case character (see here)

Other, general comments - as I usually do, I'll go through your code pretty much line by line, commenting on things as I see them

var DefaultConfig = Config{"2006-01-02"}

Now this is a perfectly valid statement. However, it makes code less readable (especially if the declaration of Config sits in a different file, or contains a ton of fields. Imagine you need ~10 config values, some of them have defaults, some of them are not. To this end, I'd recommend you explicitly mention which field you're assigning, rather than relying on the order of the fields never changing:

var DefaultConfig = Config{
    dateFormat: "2006-01-02",
}

I also don't get why DefaultConfig (a variable) is exposed, even though the field it contains isn't. The default variable clearly is an internal thing, and should as such not be exported.
The same logic applies to the Config type: if it's exported, then surely there should be some fields or methods that are exported, too. Your snippet shows no such thing.

// FromFile returns the configuration to use for did
func FromFile(path string) (c Config, e error) {

I'd opt to return a pointer, here. It forces people to check for errors. Currently, you're returning a valid Config value, which can be safely used. I can call conf, _ := FromFile("/tmp/does_not_exist"), and assume everything went fine. I'm guaranteed to receive a valid value after all. Not so if I were to change the function to:

func FromFile(path string) (*Config, error) {}

If there's an error, I'll get a nil value, accessing that will result in a runtime panic, so I'm forced to defend against that, and check errors. I'll rewrite some of the code to use a pointer return value

// c = DefaultConfig <-- this is now gone
if lines, err := File2lines(path); err == nil {

Yes, again there's a comment I can make. because your if clause is true on success, the rest of your code is essentially wrapped in a big if. Golang programmers prefer to return early, especially if something goes wrong. Rewrite this to:

// don't know why this function is exposed, don't know what this func looks like, but I prefer CamelCasing (or camelCasing) the name - also recommended style
lines, err := FileToLines(path); err != nil {
    return nil, err
}
// one less level of indentation here

Now a bigger one. Your propertyOf function has a triple return value, again this is perfectly valid. However, it's clear that you're returning a key-value pair, and an error. Why not create a type kv struct{ key, value string} or something so it's clear that the first 2 strings belong together. This is safer because it doesn't allow for people being silly and ignoring one of the return values. The key means nothing without the value and vice-versa after all. The data looks better when grouped. Anyway, let's rewrite the body of your FromFile func now:

// not in a func
type kv struct {
    key, value string
}

conf := DefaultConfig
// in func FromFile
for _, ln := range lines {
    kv, err := propertyOf(ln) // we will look at propertyOf later
    if err != nil {
        return nil, err // again, return early
    }

    // valid
    switch kv.key {
    case "dateformat":
        // maybe check for empty strings and return errors here
        // you could even try parsing a known date, to see if the format is valid
        conf.dateFormat = kv.value 
    case "some-other-key":
        // implement other values
    default:
        // optional, unknown config could just as well be skipped
        return nil, fmt.Errorf("unknown config %s (value %s)", kv.key, kv.value)
    }
}
return &conf, nil // all good

Now let's look at the propertyOf function. I must say, the name is confusing to say the least. I'd call it something like lineToKVPair or something. Then again, naming things isn't my speciality. However, when I see propertyOf, I'm expecting either a pointer to a field to be returned (through reflection), a boolean (whether or not an argument of type interface{} is a struct, and has a given property, or simply a bool given a value, telling me whether or not that value is a property of a given object.

// again: return pointer to ensure the error is checked
func toKVPair(ln string) (*kv, error) {
    parts := strings.Split(
        strings.Trim(ln, " \t\n"), // removes all spaces, tabs, and new-line characters from string in one call
        "=",
    )
    if len(parts) != 2 {
        return nil, fmt.Errorf("string %s could not be parsed to a key-value pair", ln)
    }
    return &kv{
        key: parts[0],
        value: parts[1],
    }, nil
}

Now putting all of this together, the code looks something like this:

type Config struct {
    dateFormat string
}

type kv struct {
    key, value string
}

var DefaultConfig = Config{
    dateFormat: "2006-02-01",
}

func FromFile(path string) (*Config, error) {
    lines, err := FileToLines(path)
    if err != nil {
        return nil, err
    }

    conf := DefaultConfig

    for _, ln := range lines {
        kv, err := propertyOf(ln)
        if err != nil {
            return nil, err
        }

        switch kv.key {
        case "dateformat":
            conf.dateFormat = kv.value 
        case "some-other-key":
            // implement other values
        default:
            return nil, fmt.Errorf("unknown config %s (value %s)", kv.key, kv.value)
        }
    }
    return &conf, nil // all good
}

func toKVPair(ln string) (*kv, error) {
    parts := strings.Split(
        strings.Trim(ln, " \t\n"), // removes all spaces, tabs, and new-line characters from string in one call
        "=",
    )
    if len(parts) != 2 {
        return nil, fmt.Errorf("string %s could not be parsed to a key-value pair", ln)
    }
    return &kv{
        key: parts[0],
        value: parts[1],
    }, nil
}

This, to my eye, looks a lot tidier, and simpler. But it's still not what I'd actually do. My preferred approach is even simpler, still.


Use environment/config packages.

Golang projects are more often than not either CLI tools, or containerised binaries. In case of the latter, they usually get their config from environment variables. There's a lot of packages out there that handle this really nicely.

Now for some shameless self-promotion: I usually use this package, and it served me well. So much so that I've contributed a bit to it. To do what you're trying to do is dead-easy:

import(
    "github.com/caarlos0/env"
)

type Config struct {
    DateFormat string `env:"DATE_FORMAT" envDefault:"2006-01-02"`
}

func GetConfig() (*Config, error) {
    var c Config
    if err := env.Parse(&c); err != nil {
        return nil, err
    }
    return &c, nil
}

That's it! if you want some validation to be added, you can have a look at the custom parsers, or just add them on to the Config type:

// do validation here, and call that in the GetConfig func before you return
func (c Config) validate() error {}

When you are working on a CLI tool, there's other packages you can use that handle both flags and environment variables. The most commonly used one (in my experience) is viper. It's quite nicely documented, although I find using it can lead to quite verbose code in the end.

What you can quite easily do, however, is use the standard flags package, and use os.Setenv to populate the environment variables accordingly, and then use the code above:

import(
    "flags"
    "fmt"
    "os"
)

func main() {
    // create the flag
    flagSet := map[string]string{
        "DATE_FORMAT": "2006-01-02",
    }
    flags.StringVar(&flagSet["DATE_FORMAT"], "date-format", flagSet["DATE_FORMAT"], "set different date format string")
    // parse the flags:
    if err := flags.Parse(); err != nil {
        fmt.Printf("error: %+v", err)
        os.Exit(1)
    }
    // populate env
    for k, v := range flagSet {
        if err := os.Setenv(k, v); err != nil {
            fmt.Printf("error: %+v", err)
            os.Exit(1)
        }
    }
    // load config struct from the env vars we just set
    conf, err := GetConfig()
    // etc...
}

Note: All the code I wrote, I just typed here on the site, I've not tested anything, so there might be some typo's and such, but the basic ideas are all there. I think the code is close to complete enough to be used as a basis to write your own solution.

Have fun

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.