Code review

When I want to contribute to some repository on GitHub I always check Issues section first. And this is how I’ve started with the repo I’ll discuss in this post. I’ve found an improvement request and wanted to implement it. First I’ve learnt how to do that, as I’ve not done it before. And then I’ve started to analyse the code in order to add this new feature, as I’ve only glanced at it before. But the code was really bad. I’ll not mention the repo name here, as my intention isn’t to diss the author. And I’ll try to “anonymise” the code as far as I can.

if outputFile != "" {
	emptyFile, err := os.Create(outputFile)
	if err != nil {
		log.Fatal(err)
	}
	log.Println(emptyFile)
	emptyFile.Close()

	var wg sync.WaitGroup
	for i := 0; i < concurrency; i++ {
		wg.Add(1)
		go func() {
			someFunction(someParam, verbose, outputFile)
			wg.Done()
		}()
		wg.Wait()
	}

	} else {

		var wg sync.WaitGroup
		for i := 0; i < concurrency; i++ {
			wg.Add(1)
			go func() {
				someFunction(someParam, verbose, outputFile)
				wg.Done()
			}()
			wg.Wait()
		}
	}

First thing. Why create outputFile and then close it immediately? I don’t get it. Then it logs *File which gives something like this 2021/03/27 13:38:20 &{0xc0000221e0}. Why, what information this provides?

Another thing is DRY. We could have code like below and it would give us exactly the same behaviour. But we’re not repeating ourselves.

if outputFile != "" {
	emptyFile, err := os.Create(outputFile)
	if err != nil {
		log.Fatal(err)
	}
	log.Println(emptyFile)
	emptyFile.Close()
}

var wg sync.WaitGroup
for i := 0; i < concurrency; i++ {
	wg.Add(1)
	go func() {
	    someFunction(someParam, verbose, outputFile)
	    wg.Done()
	}()
	wg.Wait()
}

Then we have someFunction which takes outputFile as parameter, but it doesn’t use it at all, see below. That’s entire function.

time.Sleep(500 * time.Microsecond)
scanner := bufio.NewScanner(os.Stdin)
for scanner.Scan() {
	link := scanner.Text()
	anotherFunc(something)
}

Moreover outputFile is a global variable. So, we don’t have to pass it to any function.

App provides verbose mode, so it has this:

requestDump, err := httputil.DumpRequest(req, true)
if err != nil {
	fmt.Println(err)
}
if verbose == true {
	fmt.Println(string(requestDump))
}

What’s wrong with that? We’re creating requestDump every time, no matter if we need it or not. It should be like this:

if verbose == true {
	requestDump, err := httputil.DumpRequest(req, true)
	if err != nil {
		fmt.Println(err)
	}
	fmt.Println(string(requestDump))
}

Also, when we’re dealing with bool we don’t have to compare it to true, so if verbose would’ve been enough. And if httputil.DumpRequest returns error it’s printed, but the program proceeds. However in documentation one can read ‘If DumpRequest returns an error, the state of req is undefined.’. And yes, req is used further on in the program.

And yet another thing:

resp, err = client.Do(req)
bodyBytes, err := ioutil.ReadAll(resp.Body)
if err != nil {
	log.Fatal(err)
}

One should first check for error and only then read from resp.Body, because otherwise our program could malfunction.

Last thing, there’s function which returns *http.Response, string,  []error. The problem is it’s used like this in the code _, body, _ := yetAnotherFunc(u.String()). As you can see only string is used. Moreover resp.Body is never closed, so we’re leaking resources and as this tool is for checking lots of URLs it’s a problem.

Function also returns []error, but it’s nowhere to be found in function’s body. Error checks appear only twice, in one case error is printed and execution proceeds. In other it’s logged with fatal, so none of these errors is ever returned.

I’m making mistakes myself. And often quite stupid, basic ones. So really, I’m not trying to play smart here. Maybe this repo will be improved in future. I probably should’ve made a PR (pull request) instead of writing this post, but there are so many mistakes (I’ve listed just the big ones) that I would basically have to rewrite the whole thing. And if you think this post is harsh then try to imagine PR changing almost every line of your code.