17 small things developers do that cause code reviews to fail

Daniel Anderson
May 2 · 6 min read
Image for post
Image for post
Photo by Pankaj Patel on Unsplash

Code review sometimes referred to as peer review, is the act of checking a fellow developers code to see if there are any mistakes and meet the standards of the business.

In my current role, I am responsible for reviewing my team’s code. Daily I come across the same common mistakes that cause the reviews to fail. Below are some examples. I’m a full-stack developer so there’s a wide range of tech, hope you enjoy!

  1. Not using switch statements

It’s as simple as using switchinstead of else if

Fail:

if (clicks == 0) 
{
newUser();
}
else if (clicks == 1)
{
returningUser();
}
else if (clicks == 2)
{
almostFinished();
}
else if (clicks == 3)
{
finished();
}
else if (clicks == 4)
{
restart();
}

Pass:

switch (clicks)
{
` case 0:
newUser();
break;
case 1:
returningUser();
break;
case 2:
almostFinished();
break;
case 3:
finished();
break;
case 4:
restart();
break;
}

2. Adding unnecessary else statements

A small one but just return the value.

Fail:

if (id == 0) 
{
return null;
}
else
{
return success();
}

Pass:

if (id == 0) 
{
return null;
}
return success();

3. No comments on interfaces

I mean you could argue there’s a lot of places you should add comments but with interfaces, there’s no excuse! This also has the benefit of adding /// <inheritdoc /to the class that will be inheriting the interface.

Fail:

interface IQueue
{
bool Proccess(string message);
bool Close();
}

Pass:

/// <summary>
/// This interface is awesome
/// </summary>
interface IQueue
{
/// <summary>
/// This method is awesome
/// </summary>
/// <param name="message">The awesome parameter</param>
/// <returns>A awesome bool</returns>
bool Proccess(string message);

/// <summary>
/// This method is awesome as well
/// </summary>
/// <returns>A awesome bool</returns>
bool Close();
}

Inherit doc :

public class Queue : IQueue
{
/// <inheritdoc />
public bool Proccess(string message)
{

4. Renaming WCF markup file

If you’ve ever worked with WCF, you will know this can cause problems if it doesn’t match up with the web config.

This usually happens when developers copy an existing WCF service file (.svc) to create a new one but they only change the service name file without changing the WCF service markup.

The markup file usually looks like something below:

<%@ ServiceHost Language="C#" Debug="true" Service="WebSrv.MyService" Factory="Tools.Services.ServiceHostFactory`1[WebSrv.MyService]"  CodeBehind="MyService.svc.cs" %>

5. Missing information App / Web Config

If you’ve ever worked with .net this can cause issues when releasing to live. This can be a missing service reference or NOT adding live information into the release transform config.

6. Messing up the project file

This happens a lot in a .net project, usually when not getting/pulling the latest version of the code on a regular basis and the knock-on effect causing a messy merge.

7. Minimal test asserts

In my experience when creating tests you should really check a lot of aspects of your code to help your application grow.

Ideas for asserts

  • Database data is what is expected

8. Not checking Nulls

NULLS can cause a lot of errors! It is important to have the right checks in place to stop those object ref errors!

Fail:

var account = _accountRepository.GetAccountByID(accountID);if (validateEmailAddress(account.emailAddress))
{

Pass:

var account = _accountRepository.GetAccountByID(accountID);if (account == null) 
{
return Error("Account not found");
}
if (validateEmailAddress(account.emailAddress))
{

9. Not using enums

Enums help with readability and can help make changes in the future a lot more efficient.

Fail:

var contracts = GetContractsByStatus(2);

Pass:

var contracts = GetContractsByStatus(ContractStatusEnum.Active);

10. Not using named parameters when passing in NULL for optional parameters

Like the enum example, the below can really help with readability.

Fail:

var contracts = SearchContracts(null, lastWeek, null);

Pass:

var contracts = SearchContracts(status: null, startDate: lastWeek, maxNumber: null);

11. Unclear variable naming

This can go for everything. Methods, classes. Etc..

Fail:

var x = false;

Pass:

var isValid = false;
Image for post
Image for post
Photo by Franki Chamaki on Unsplash

12. SELECT*

If you have ever worked with databases, you will know what I mean! Especially when the table has a lot of columns.

Fail:

SELECT*
FROM acc.Account a
WHERE a.AccountID = @AccountID

Pass:

SELECT a.EmailAddress,
a.StatusFK
FROM acc.Account a
WHERE AccountID = @AccountID

13. Bad SQL formatting

SQL is not the easiest language to read, and as the queries get larger and more complex the formating becomes extremely important for readability. Everyone has their own style so this can divide opinion in my experience.

Fail:

SELECT id, FirstName, LASTNAME,c.nAme FROM people p left JOIN cities AS c on c.id=p.cityid;

Pass (maybe) — still nicer:

SELECT p.PersonId,
p.FirstName,
p.LastName,
c.Name
FROM [dbo].[person] p
LEFT JOIN [dbo].[city] c
ON p.CityId = c.CityId;

14. Not adding SQL scripts to source control!

There is nothing worse than when you’re creating a release plan and then realising that SQL scripts are missing from the source control! It’s vital to catch these in reviews.

Image for post
Image for post
Photo by Valery Sysoev on Unsplash

15. Inline CSS

The CSS needs to be separated from the HTML otherwise this can get messy very quickly.

Fail:

<div style="font-size:18px;color:#C8C8C8;padding:20px;margin-top:5px;">

Pass:

CSS:

.header-wrapper {
font-size: 18px;
color:#C8C8C8;
padding:20px;
margin-top:5px;
}

HTML:

<div class="header-wrapper">

16. Not using === in Javascript

If you have coded in Javascript you will know what I mean!

Fail:

if (isValid == true) {

Pass:

if (isValid === false) {

17. Leaving console logs

Usually, these are just left in there because the developer has needed them for debugging! Be careful with these as it can lead to showing business information to the client.

console.log(123);
console.log(error);
console.log("hello world");

18. Using “any” in TypeScript

You can change the tsconfig.json file so using any types makes the build fail. You do this by setting"noImplictAny": true

Fail:

createNewUser(userDetails: any) {

Pass:

createNewUser(userDetails: user) {

19. Using !important in CSS to override styles

This can get confusing very quickly! When I see !imporanttoo much, it usually means there’s an issue with the HTML or CSS.

.active {
font-size: 25px !important;
color: green !important;
}

Conclusion

Failing code reviews are almost impossible to stop and that’s fine! It is important to look at the smaller details in your code to create a scalable and easy to understand an application.

Ways to cut down failing code reviews:

  • Coding standards: for each tech stack you use

A note from JavaScript In Plain English

We have launched three new publications! Show some love for our new publications by following them: AI in Plain English, UX in Plain English, Python in Plain English — thank you and keep learning!

We are also always interested in helping to promote quality content. If you have an article that you would like to submit to any of our publications, send us an email at submissions@plainenglish.io with your Medium username and we will get you added as a writer. Also let us know which publication/s you want to be added to.

JavaScript In Plain English

New JavaScript + Web Development articles every day.

Medium is an open platform where 170 million readers come to find insightful and dynamic thinking. Here, expert and undiscovered voices alike dive into the heart of any topic and bring new ideas to the surface. Learn more

Follow the writers, publications, and topics that matter to you, and you’ll see them on your homepage and in your inbox. Explore

If you have a story to tell, knowledge to share, or a perspective to offer — welcome home. It’s easy and free to post your thinking on any topic. Write on Medium

Get the Medium app

A button that says 'Download on the App Store', and if clicked it will lead you to the iOS App store
A button that says 'Get it on, Google Play', and if clicked it will lead you to the Google Play store