Help me refactor my code

Hi all,

I’m a few hours into learning Go, and I’m loving it. My programming history has been C++ > Java > Groovy > Go. I mostly use programming for simple tooling, so I have to admit I’ve not reached pro at any of them, but I do try to take an interest in good habits/best practice.

With that in mind I would like to understand the motions you guys would go through if you were to refactor my rather basic project.

In this example I’m taking a files contents (called LICENSE) and adding it to my go source code as a header.

// USAGE: go run main.go -lp=/some/file.txt -sp=/a/directory/with/go/files

package main

import (
	"flag"
	"fmt"
	"io/ioutil"
	"os"
	"path/filepath"
	"strings"
)

var lpPtr string
var spPtr string

func init() {
	// Create some flags for the user
	flag.StringVar(&lpPtr, "license_path", "LICENSE", "path to LICENSE file")
	flag.StringVar(&lpPtr, "lp", "LICENSE", "path to LICENSE file (Short hand)")
	flag.StringVar(&spPtr, "source_path", "demo.txt", "path to source file")
	flag.StringVar(&spPtr, "sp", "demo.txt", "path to source file (Short hand)")
	flag.Parse()
	fmt.Println("License Path:", lpPtr)
	fmt.Println("Source Path:", spPtr)
}

var licenseData []byte

func main() {
	// Load the LICENSE into memory
	var err error
	licenseData, err = ioutil.ReadFile(lpPtr)
	check(err)

	// Walk the directory structure, calling license at each node
	err = filepath.Walk(spPtr, license)
	check(err)
}

// Check for error and if error, then panic
func check(e error) {
	if e != nil {
		panic(e)
	}
}

// license is called for each file visited by Walk. If its not a directory 
// and a file with suffix '.go' then it will add the License via licenseWriter
func license(path string, f os.FileInfo, err error) error {
	if !f.IsDir() {
		if strings.HasSuffix(path, ".go") {
			fmt.Printf("Filename: %s \n", path)
			addLicenseHeader(path, licenseData)
		}
	}
	return nil
}

func addLicenseHeader(path string, data []byte) {
	//err =: ioutil.WriteFile(path, data, 0644)
	//check(err)
	fmt.Printf("License added to: %s \n", path)
}

I realise there’s a lot wrong with this. Global scope vars as one example, but I’m keen to know what you think (be nice :wink: ).

I would also like to better understand testing in Go, so any pointers around that would be welcomed too!

Thank you kindly in advance!

Welcome to Go. I hope your stay is a lengthy one :smile:

Actually it seems pretty reasonable, even the “global variables”.

First of all, they are not global, they are package level, which greatly restricts them. Second of all, you are defining a main package. There will only be one, and it will not be used by other packages. So they are perfectly reasonable - no need to make it more complex.

I cannot find and bad things in your code. Of course there are some usability choices, like just taking two fixed parameters instead of -source_path=something. Instead of panic, just print the error and os.Exit(1), but again, extremely small things.

You are on the right track - coming from Java I will expect the biggest challenge to be not to over-complicate things, and what you are showing seems perfectly reasonable.

4 Likes

Thanks for the words of encouragement.

In the past I’ve been guilty of trying to understand everything about a language before writing a single line of code. I’ve not done that with Go, and its paid off because of it.

I did, however, find the following article which really helped me better understand interfaces and their uses, and also architecting a Go program.

Applying The Clean Architecture to Go applications

You code is mostly fine. There are two things that strike me as un-Go and that are quite commonly seen in code from new Go practitioners (and then not much after that):

There’s no need to point out that a variable is a pointer, and these are in fact not pointers. licensePath and sourcePath would be expected; as they are package level, longer names are preferable as it may not be obvious from the context.

I consider this an antipattern. It’s not obvious at the call site what check(err) does (although we can guess it doesn’t return if the error is non-nil), and it doesn’t promote good error handling. In this case - as it’s a small CLI program - exiting may be appropriate on error but you should still give the user some context. If this were a library, you’d want to return or amend and then return the error as appropriate.

licenseData, err = ioutil.ReadFile(licensePath)
if err != nil {
    log.Fatal("Reading license:", err)
}

Now both the user and your fellow programmer knows exactly what happens in the error case, and it’s just one line and a brace longer. Any Go programmer’s eye is trained to automatically recognize the if err != nil case so the mental effort when reading is actually less than the check() call.

(You also need an error check at the start of license(); if the file or directory is not readable, your function may be called with f==nil and a non-nil err.)

1 Like

@calmh, thanks for taking the time to review and comment. I learnt a valuable lesson here because the check function was something I picked up on from GoByExample. Equally, the hungarian notation was also taken from the same resource.

Again, thanks for taking time out to comment. :slight_smile:

1 Like

Huh. Yeah, I don’t think that’s super idiomatic. It’s not that there’s anything wrong with calling something ...Ptr per se, I have a couple places I do that as well, but that’s mostly when I already have a foo and for some reason need/want to assign &foo to something and either have both in scope or otherwise differentiate between them.

I’m guessing it might be in that example to highlight the fact that flag.Int() and friends return a pointer to the actual value.

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.