TL;DR: add code comments and keep line lengths short
Lately I’ve been digging into Meshery trying to get one of the adapters working. In doing so, I’ve seen opportunities we could improve the code. As projects grow, it is important to try and keep the code as clean and easy-to-read as possible. This is even more important on an open-source project with many people working on it. Here are a couple friendly reminders to those contributing code.
Add informative code comments.
This is especially important in this type of project as it will help new people get up to speed and understand the code faster. If you think something isn’t clear or requires some context, add a comment.
For example, can you tell what this does just by looking at it?
if len(args) > 0 {
r, _ := regexp.Compile(`\W`)
meshName = r.ReplaceAllString(strings.ToUpper(strings.Join(args, "_")), "_")
}
It’s a simple bit of string formatting, but if you are new to the project or the language, a comment makes it much easier understand what it does and, more importantly, why.
if len(args) > 0 {
// if a mesh name was provided, convert it the same format as adapter.Name
// all args are joined, converted to upper case, and non-word characters replaced with "_"
// examples:
// args = Linkerd -> ["LINKERD"] -> "LINKERD"
// args = nginx service mesh -> ["nginx", "service", "mesh"] -> "NGINX_SERVICE_MESH"
r, _ := regexp.Compile(`\W`)
meshName = r.ReplaceAllString(strings.ToUpper(strings.Join(args, "_")), "_")
}
Keep line lengths short.
Reading code is hard. It’s even harder when you have to scroll sideways to see what a particular bit of code is doing. Breaking long lines into smaller ones makes code much easier to read, especially for those working on small screens like laptops.
For example, is this easier to read?
return errors.New(ErrGettingLatestReleaseTagCode, errors.Alert, []string{"Could not fetch latest stable release from github"}, []string{err.Error()}, []string{"Failed to make GET request to github", "Invalid response recieved on github.com/<org>/<repo>/releases/stable"}, []string{"Make sure Github is reachable", "Make sure a valid response is available on github.com/<org>/<repo>/releases/stable"})
Or is this easier?
return errors.New(
ErrGettingLatestReleaseTagCode,
errors.Alert,
[]string{"Could not fetch latest stable release from github"},
[]string{err.Error()},
[]string{
"Failed to make GET request to github",
"Invalid response received on github.com/<org>/<repo>/releases/stable",
},
[]string{
"Make sure Github is reachable",
"Make sure a valid response is available on github.com/<org>/<repo>/releases/stable",
},
)
I’m sure most people will agree that the second one is easier to read and understand.
The max line length is personal preference, but typically the max line length it 80 characters (here’s a post about it if anyone is interested). However, there are times where this isn’t practical. In most projects I work on, the max line length is 120. I’ve found several lines of code in this project that are 2-300 or even more. Enforcing line length would result in quite a bit of work across all the components, so it isn’t really feasible to add the enforcing now. However, when writing new code or working on existing code, we can all do our part to make it nicer for those who come after us.
Makes much sense. To your point, it’s not about personal preference near as much as it is about consistency, which leads to ease of legibility and contribution (great point about how much more important comments and docs are in open source projects that by their very nature have transient contributors), higher quality (less bugs both in that logic is better understood by those that didn’t write the code and in that when the author has to articulate in written word what they have codified, that activity serves as something of a review in and of itself), and in turn, into higher project velocity.
However, decreasing it to a more reasonable limit would have required a large amount of changes to an already large PR. To prevent overly-inflating the change-set, I opted to leave it. Maybe down the road this could be updated.
