Code Review Comments General

Code Review Comments

This page aims to support common code review comments and feedback to open-mSupply code, and related projects.

As a code reviewer, we recommend you point to the explanations here when referring to a common code review feedback. If you find yourself making a similar comment regularly add it here so we can improve our knowledge base and support new developers learning without repeating yourself too often.

Prefer a simple solution over a complex one

Experienced Developers often argue for a simple solution rather than a complex and re-usable approach. Why? Complex code is hard to read, debug and understand, which means it can take time to review, test and is more likely to contain errors.

We often end up with overly complex or complicated code when...

• Trying to anticipate future requirements that may never come
• Trying to optimise code performance
• Lack of experience with the code base, or time pressure

If one of these factors is encouraging you to choose a complex solution, it might be worth discussing your solution with another developer to get another brain on the task.

Obviously sometimes we need complex code, and some problems are inherently complex, it isn't always possible to simplify.

When using this comment, it should always come with a suggestion on how to simplify the code.

Too Complex

type stringTransformer {
     stringTransformerFunction: fn(s:string):string
}

fn uppercaser = (str:string, num_chars:number):string {
 let return_str = "";
 let i = 0;
 while(i < num_chars){
   return_str += str.charAt(i).toUpperCase();
 }
 return return_str + str.slice(i);
}

fn uppercaseFirstLetter(str:string): string{
   let transformFunction = stringTransformer{ stringTransformerFunction: uppercaser }

   return transformFunction.stringTransformerFunction(str, 1);
}

Suggestion "It's unlikely we'll need to uppercase anything but the first character of the string so a simpler solution like this could be considered."

function uppercaseFirstLetter(str: string): string {
  return str.charAt(0).toUpperCase() + str.slice(1);
}

Note: Simplicity can be subjective, so take care when using this comment. Remember to respect other developers ideas and code, even if someone has gone about a change differently to what you might have, it doesn't mean it's bad or wrong.

See also: https://go.dev/talks/2015/simplicity-is-complicated.slide#11

Nit's Don't need to be actioned

Comments are often prefixed with nit: this represents something a reviewer has noticed about the code, but doesn't think is important to enough to stop the code from being merged. Often used for spelling mistakes in comments for example.

Reviewers may want to commit changes fixing spelling errors to a review branch, rather than polluting the PR Comments with lots of spelling improvements. Obviously if the spelling correction isn't obvious, or might require associated code changes, a PR Comment might be more approriate.