Software development is like nature: it's in a constant state of flux and continuously changing. As software engineers, it's our responsibility to embrace the flux and write clean code that is optimized for change. In doing so, we can rapidly innovate and satisfy customer needs.
- Development Best Practice
These principles provide a guide for designing software. At times they may seem contradictory, but they ultimately aim to produce the same kind of code: simple and maintainable.
"Debugging is twice as hard as writing the code in the first place. Therefore, if you write the code as cleverly as possible, you are, by definition, not smart enough to debug it." - Brian W. Kernighan
- SOLID - The Single-responsibility Principle, Open-closed Principle, Liskov Substitution Principle, Interface Segregation Principle and Dependency Inversion Principle collectively define a guide for writing maintainable code. While often referenced in the context of an object-oriented language, they are applicable to all languages.
- LoD - The Law of Demeter tells us a construct should talk to their direct dependencies and only their direct dependencies (don't talk to strangers). Reaching to transitive dependencies creates complex layers of interaction that drive toward spaghetti code.
- KISS - "Keep it simple, stupid" was coined by the US Navy. Systems should be designed as simply as possible.
- YAGNI - "You aren't gonna need it" if you don't have a concrete use-case, don't write it.
- DRY - "Don't repeat yourself" suggests you should preference code re-use over duplication. However, you should avoid hasty abstractions as the wrong abstraction can be extremely costly to correct. Instead, lean into small amounts of duplication to help identify the right abstractions through multiple use cases and define the pathway to DRY design.
"Leave code better than you found it"
Generally, you want to fix problems as they're identified. This includes micro refactors to accommodate new code that causes existing abstractions to, possibly, no longer make sense. Resist bolting on new code that is uncomplimentary to existing code. Refactoring efforts generally belong on their own PR thereby following the SRP guideline for PRs.
Finally, we have delivery commitments. Larger more intentional refactors may be required to fix a problem you've identified and attempting to tackle it may delay your deliverable. In such a case, write down your ideas and start a conversation with the wider team.
What constitutes clean code? We want simple and maintainable, but how do we define that? The Go community has developed a set of resources that can be considered part of our clean code definition. Take the time to read them diligently and use them as references in code reviews when providing feedback.
| Title | URL | Description |
|---|---|---|
| Effective Go | https://go.dev/doc/effective_go | Official Go documentation on writing Go code |
| Code Review Comments | https://github.com/golang/go/wiki/CodeReviewComments | Community driven additions to Effective Go that are battle tested |
| Practical Go | https://dave.cheney.net/practical-go/presentations/gophercon-israel.html | Dave Cheney is a respected member in the Go community that has established further guidance based on experience |
| Style Guide | https://google.github.io/styleguide/go/decisions | Google developed a style guide that is referenced by the Code Review Comments |
The Additions section provides additional guidance for the EKS Anywhere code base that isn't covered by these articles or proves contradictory.
Shared mutable state is believed by many to be the “root of all evil”, or at least the cause of most of the accidental complexity in our code. And “Complexity is the root cause of the vast majority of problems with software today.” - Mauro Bieg
Package level state is rarely required. If it is, it should be composed of re-usable constructs defined and exposed from the package itself.
See https://dave.cheney.net/practical-go/presentations/gophercon-israel.html#_avoid_package_level_state).
This section is included because the phrase surfaces regularly.
Code Review Comments provides a simple explanation that covers the general case. However, we want to stress this is the general case, not all cases.
When developing abstractions it may become necessary to return interfaces but this should be considered orthogonal to accepting interfaces.
See https://github.com/golang/go/wiki/CodeReviewComments#interfaces
Unit tests should:
- focus on the public API of a package.
- prefer the
_testwhen testing the public API of the package. - treat the package as a black box.
Test names:
- When testing a function, the test name should start with the name of that function.
- When testing a method, the test name should start with the name of the receiver type followed by the method name.
When testing the same name using different test funcs append a concise test description to the test names.
If you public API is hard to test, consumers may find it hard to use - consider restructuring your package API.
Prefixing error messages with the word error is typically unnecessary leading to error bloat when adding context; leave prefixing to presentation logic.
Also see https://github.com/golang/go/wiki/CodeReviewComments#error-strings.
We consider boolean parameters an anti-pattern. They indicate toggling behavior within an algorithm suggesting the algorithm has side effects and it makes rationalize behavior at the site of consumption difficult.
For example
parsed := p.Parse(input, false) // What does false mean? What would happen if it were true?At minimum, exported APIs should be documented. Consider go doc output when documenting APIs. Consumers of the APIs can see the function signature, not the implementation. Focus comments on what the function provides as opposed to the nitty gritty details of the implementation.
See https://github.com/golang/go/wiki/CodeReviewComments#doc-comments and https://github.com/golang/go/wiki/CodeReviewComments#package-comments.
Naked returns are unnecessary, just be explicit.
See https://github.com/golang/go/wiki/CodeReviewComments#naked-returns.
The need to panic only arises when the program enters an irrecoverable state. All other cases, especially when writing self contained packages, should return their errors.
See https://github.com/golang/go/wiki/CodeReviewComments#dont-panic.
A replace directive replaces the contents of a specific version of a module, or all versions of a module, with contents found elsewhere. The replacement may be specified with either another module path and version, or a platform-specific file path.
Avoid using replace directives for long periods of time. If a replace directive is required, an exit plan to remove the replace is also required.
Traditionally, many coding standards have stipulated 80 chars as the maximum line length. It is thought to originate from the days of punch cards where IBM used 80 column widths. The width translated well to small width terminal monitors hence was adopted in the early days of computing.
In the present day we still see 80 chars line length feature in coding standards. However, much has changed since the standard was originally employed. Attempts, through research, to pin down the optimal line length exists but have resulted in conjecture and contradiction. For this reason, we consider other use-cases and constraints to help decide line length:
- laptops with less than 16" wide monitors are in abundance.
- side-by-side comparison of files is useful.
- horizontal scrolling is annoying.
- some developers need larger font sizes.
When developing code, we ask you to take these points into consideration and not to create obnoxiously long lines. Compliment existing code. Configure your IDE to plot margins so it may aid your decision. A good rule of thumb is the 100 characters mark give or take 20.
See https://github.com/golang/go/wiki/CodeReviewComments#line-length.
All Go source files should be named with snake_case including files that represent types.