Code review needed

I wrote my first bigger Go program for learning Go:

Would be cool if someone could take a look at it and tell me if it is idiomatic Go code, or what I can improve. I also think instead of “action=name” it would be better to use just “name”. Is this possible with “flag”?

I also noticed that I can ignore the error returns from functions. This looks odd to me, since Go is pretty picky with other things, like compiler errors for unused variables. But just ignoring returned errors is fine?

1 Like

Hi, Frank, welcome to the Go Forum!

The flag package - flag - Go Packages is only for defining flags, like --action=name, but it does give you flag.Args to retrieve the arguments after all of the flags, so you could parse positional (“unflagged”) arguments with that. Any arguments that you change from flags to positional would no longer show any documentation automatically. You could get around that by overriding the flag.Usage variable with your own that does something like this:

package main

// ...

func init() {
    defaultUsage := flag.Usage
    flag.Usage = func() {
        defaultUsage()
        fmt.Println(`positional arguments:
action
    The name of the action to perform`)
    }
}

It is possible to ignore errors, but usually you shouldn’t because if something does go wrong and an error is reported, but ignored, then the program will carry on like nothing is wrong and you might get bad results.

I have a few suggestions:

  1. Like you already asked, I would recommend not ignoring errors. Even if you just put if err != nil { return err } everywhere, at least you’ll know when something is wrong and needs to be fixed. For command line applications, sometimes I’ll just write if err != nil { panic(err) } which is considered unidiomatic by some, but useful for non-daemon command line applications.

  2. goto is unidiomatic and should be avoided. I’ve seen it used in complicated loops when a condition in a nested loop is hit needs to break several layers into an outer loop and then continue the outer loop, etc., but your usage here doesn’t need it. I would instead recommend factoring the Usage label into its own function and return when there’s an issue:

    func main() {
        // ...
        if *output_filename == "" {
            usage()
            return
        }
    }
    
    func usage() { /* ... */ }
    
  3. You can also invert your if len(os.Args) > 1 condition:

    if len(os.Args) <= 1 {
        usage()
        return
    }
    

    So that you can un-indent the rest of your code in main

  4. You can use io.Copy or io.CopyN to copy data from one file to another instead of writing your own appendFile function.

  5. I recommend using the constants in the io package, e.g., io.SeekEnd instead of the “magic number” 2 when seeking from the end of files.

  6. This might not be an issue for your use case, but: math/rand isn’t cryptographically secure and if you want something secure, you should use the crypto/rand package. math/rand is deterministic and will generate the same pseudo-random numbers every time because its source uses the same seed.

  7. You could use rand.Read to store random data right into your bytes slice in the random function instead of calling rand.Uint32 and truncating to only 8-bits at a time.

Thanks, I’ve updated it. I think now it is a nice little tool. Unlike the “embed” feature of Go, it supports files bigger than 2 GB as well.

BTW, I noticed in the documentation, that the error check is written in one line, as part of the if statement:

if _, err := io.CopyN(os.Stdout, r, 4); err != nil {
		log.Fatal(err)
}

I think an extra line is easier to read. What is the recommended way for Go?

I think it’s common to see this code exactly as you have it written here, but I wouldn’t call it “unidiomatic” to write it as

_, err := io.CopyN(os.Stdout, r, 4)
if err != nil {
    log.Fatal(err)
}

Neither form is harder to read than the other :man_shrugging: