Several concerns about the document

[benhoyt]

I appreciate the effort that went into this, however, I think there are several unnecessarily opinionated views as well as some problematic things.

You start by advocating Test-Driven Development, which is somewhat controversial. Not everyone likes writing their tests first (I don't), and usually I find that only by playing with the code do you know what your contract will be. You could argue that the best Go developers are the core Go implementors, and you won't find them using or talking about TDD much.

"Unnecessary comments are the biggest indicator of code smell." Unnecessary comments are a problem, sure, but the "biggest" indicator of a code smell? That seems unlikely.

I disagree that 5-8 lines is the ideal length for a function. As long as it's readable and testable and "does one thing", it can be many more lines. Code Complete for example says almost the opposite: "The routine should be allowed to grow organically up to 100-200 lines, decades of evidence say that routines of such length no more error prone then shorter routines."

I think your first refactoring of GetItem to push the code to the left is great. However, then you splits it into 4 tiny functions, and in my opinion it just makes the code more verbose and harder to read.

"Here's a good rule of thumb: If the value, err := pattern is repeated more than once in a function, this is an indication that we can split the logic of our code into smaller pieces." -- I disagree that this is a good idea, and you'll see many counter-examples in well-written Go code, including the standard library.

"On the other hand, when we split up our functions like we did above, it becomes much easier to get 100% code coverage because we're dealing with functions that are maybe only 4 lines each (when written by a sane person), as opposed to 400. That's just common sense." -- but I believe unit testing should be testing units, i.e., the GetItem function, not all the little helpers.

"Instead, we can turn the Cache property of our App structure into a private property and create a getter-like method to access it. This gives us more control over what we are returning; specifically, it ensures that we aren't returning a nil value:" -- that pattern can be useful sometimes. However, your Cache() method is not usable concurrently, or at least, it may create two NewKVCache() instances. Need to use a lock to make it concurrent safe.

In the section on interfaces, it seems like you don't understand embedding, and your comments are opinionated and disparaging the Go compiler implementors (not a good look IMO). "Personally, I think that this is a massive oversight in the Go compiler. This code should not compile... but while this is being fixed (assuming it ever will be)..." No, this is not an oversight, this is a feature. The code you give:

type NullWriter struct {
    Writer
}

is equivalent to:

type NullWriter struct {
    Writer Writer
}

which makes it clear that Writer is actually just a field of the struct that happens to be an io.Writer interface, and the interface value is nil. This is actually useful in many dynamic cases, e.g., mocking only a few functions of an interface.

"This type of code should never reach production. Remember: Go does not (yet) support generics. That's just a fact that developers must accept for the time being. If we want to use generics, then we should use a different language that does support generics rather than relying on dangerous hacks." -- well, true that Go does not yet support generics. But you sound very sure of yourself here. But in Ruby and Python, everything is essentially dynamically typed, i.e., equivalent of interface{}, and Ruby and Python developers get along fine.

Have you read the following more official documents and read a lot of high-quality Go code, including the standard library? They might help you formulate your ideas better:

• Effective Go: https://golang.org/doc/effective_go.html
• Go Code Review Comments: https://github.com/golang/go/wiki/CodeReviewComments
• The Go Spec (surprisingly readable): https://golang.org/ref/spec

[Pungyeon]

Hi Ben,

First of all, thank you so much for taking the time and effort to send feedback on the article. It's very appreciated! 🙏

"Unnecessary comments are the biggest indicator of code smell." Unnecessary comments are a problem, sure, but the "biggest" indicator of a code smell? That seems unlikely.

Yeah, I agree that this is a little exaggerated :) The point I'm trying to make, is that if you need to explain your code (to yourself of others), in prose, using comments. Your code is smelly. I'll tone it down to something more appropriate... "Unnecessary comments, is a big indicator of code smell." ?

"On the other hand, when we split up our functions like we did above, it becomes much easier to get 100% code coverage because we're dealing with functions that are maybe only 4 lines each (when written by a sane person), as opposed to 400. That's just common sense." -- but I believe unit testing should be testing units, i.e., the GetItem function, not all the little helpers.

Good spot. This point is made poorly, this should definitely be rewritten.

"Instead, we can turn the Cache property of our App structure into a private property and create a getter-like method to access it. This gives us more control over what we are returning; specifically, it ensures that we aren't returning a nil value:" -- that pattern can be useful sometimes. However, your Cache() method is not usable concurrently, or at least, it may create two NewKVCache() instances. Need to use a lock to make it concurrent safe.

Alright, I will add a lock to the example code, to make this clear.

Code Complete for example says almost the opposite: "The routine should be allowed to grow organically up to 100-200 lines, decades of evidence say that routines of such length no more error prone then shorter routines."

Could you link to the evidence which they cite in the book? Would love to read it.

Some of the other points that you make, we seem to disagree fundamentally on:

I don't think that TDD is controversial. Some people hate it, but I don't think that makes it controversial. It's a pretty well established practice, and something I believe belongs in any document on writing clean code.

I disagree that this is a good idea, and you'll see many counter-examples in well-written Go code, including the standard library.

I don't think that the standard library is a good source of finding clean code.

In the section on interfaces, it seems like you don't understand embedding, and your comments are opinionated and disparaging the Go compiler implementors (not a good look IMO).

I actually agree that I am far too harsh in this section, and will change the tone of it. I actually thought I had already removed this... but I suppose not. However, I will not back down on the fact that my opinion on interface embeddings are negative. I know that it makes testing easier... but we are already being encouraged to make interfaces as small as possible, so when would this actually be useful, if we are following Go idioms? I am open to have my opinion changed, but I will need a good example and not simply that it makes mocking slightly easier....

Again, thank you very much for the feedback, I will do what I can to get the changes implemented, next time I have some spare time 😄

[AleksandrHovhannisyan]

Here are some of my suggestions for cleaning up the text:

I'd like to first address the topic of commenting code, which is an essential practice but tends to be misapplied. Unnecessary comments can indicate problems with the associated code, such as the use of poor naming conventions. However, whether a particular comment is "necessary" is somewhat subjective and depends on how legibly the code was written. For example, the logic of well-written code may be so complex that it requires a comment to clarify what is going on. In that case, one might argue that the comment is helpful and therefore necessary.

...

On the other hand, when we split up our GetItem function into several helpers, we make it easier to track down bugs when testing our code. Unlike the original version, which consisted of several if statements in the same scope, the refactored version of GetItem has just two branching paths that we must consider. The helper functions are also short and digestible, making them easier to read.

Regarding this bit:

Code Complete for example says almost the opposite: "The routine should be allowed to grow organically up to 100-200 lines, decades of evidence say that routines of such length no more error prone then shorter routines."

I've personally written some functions in the past that have breached the 100-line mark, often when prototyping or trying to test an idea to see if it would work. However, I disagree that functions in the 100-200-line range are no more error prone than their short counterparts. With longer functions, you run the risk of stuffing complex logic into a single scope that could easily be broken up into smaller, more understandable, and well-named helper functions.

[benhoyt]

Thanks for the response. Appreciate the tweaks.

Could you link to the evidence which they cite in the book? Would love to read it.

I have an e-copy of Code Complete; I'll email you privately a PDF of the two pages that are relevant here, with references to the studies.

I don't think that TDD is controversial. Some people hate it, but I don't think that makes it controversial. It's a pretty well established practice, and something I believe belongs in any document on writing clean code.

Perhaps this is a matter of definitions. If TDD is defined is writing your tests before your code, and controversy is defined as "likely to give rise to public disagreement", then yes, TDD is controversial. You can argue it's a good thing or the best way to code, but it's definitely likely to give rise to strong disagreement -- just today we had a long Slack thread at work with some engineers on each side of the "you should write tests first" fence.

I don't think that the standard library is a good source of finding clean code.

Hmm, why do you think think the standard library wouldn't be a good source of clean code? From my observation the stdlib is very high quality and generally easy to follow; the Go authors know what they're doing.

[Pungyeon]

I have an e-copy of Code Complete; I'll email you privately a PDF of the two pages that are relevant here, with references to the studies.

Thanks, that's perfect.

You can argue it's a good thing or the best way to code, but it's definitely likely to give rise to strong disagreement -- just today we had a long Slack thread at work with some engineers on each side of the "you should write tests first" fence.

Fair enough. However, I can tell you that the topic of clean code, is always guaranteed to spark some kind of strong emotion. Whether this is about TDD, no. of functions lines, variable naming etc... I knew that people were going to disagree with the article, regardless of the content, which is partially why I decided to write it tongue-in-cheek to begin with.... I want this to be a community project and I thought that provoking people a little to begin with, would probably give me more feedback than otherwise. Down the line, I want the article to become less my opinion and more the general community opinion.... so again, thank you so much for your feedback. It's really very appreciated.

Hmm, why do you think think the standard library wouldn't be a good source of clean code? From my observation the stdlib is very high quality and generally easy to follow; the Go authors know what they're doing.

They definitely know what they are doing, but I don't think that they are particularly concerned about writing clean code. Looking at some of the libraries, particularly the older libs, there is some horrible spaghetti to be found in there. I know I sound like I don't have much respect for the go maintainers, but I really do. I think they're are doing a really great job.

[benhoyt]

Cool, thanks.