Review my code please


Code review is a necessity, never just a nice to have. Why do I say that? Because it has helped me grow a lot as a programmer. When I started as a junior programmer, I’ve had my code reviewed by many senior colleagues, and I have learnt so much from them. As my experience grew, I took on the role of reviewing the junior programmers, and imparted my knowledge and good practices unto them. There are many benefits to be gained from code reviews, such as:

  • Code reviewers help junior programmers learn and grow, as well as enforce good programming habits. If you don’t review their code, you will most probably end up fixing / refactoring their code if quality is bad.
  • Getting another pair of eyes looking at your code helps to find bugs. Sometimes explaining what your code does to another person helps you realize you did something VERY wrong.
  • It’s a great form of knowledge exchange, not only just in terms of programming, but also business rules and functionality.
  • Good form of team bonding and interaction with peers.
  • Quality control…nuff said.

Having been through my fair share of reviewing and being reviewed, I thought I would share some of the common pitfalls I have encountered and also been guilty of.

Returning expressions

This is something I see quite frequent. Consider this snippet of code.

public bool CheckSomeValue(int value)
{
    if (value > 1)
        return true;
    return false;
}

This is redundant code. There’s no need to check using the if loop, just return the expression, like so.

public bool CheckSomeValue(int value)
{
    return value > 1;
}

Use the ?: Conditional Operator if possible

This is just a pet peeve of mine. I like to swap if-else condition with the shorter ?: Conditional Operator. I find it more elegant and easier to read, if the check is not complicated.

public string CheckSomeValue(int value)
{
    return (value > 100 ) ? "Greater than 100" : "Less than 100";
}

Null Coalescing is your friend

Nulls are not fun to play with, but there are ways to handle that. I’m inclined to believe that null objects can become evil when misued. In C#, the null coalescing operator is very useful when you need to specify an alternative value if your object is null. For example,

string message = null;
string result;

// crappy way
if (message == null)
    result = "Message is null";
else
    result = message;

// using null coalescing
result = message ?? "Message is null";

Optimize your strings

  string result = str1 + str2 + str3;

That line of code is bad, very bad. Strings are immutable, so essentially you have allocated way too much memory than you should have. Use string.Concat instead.

  string result = string.Concat(str1, str2, str3);

Don’t compare strings like this.

  str1.ToLower() == str2.ToLower()

Instead, use string.Compare

  string.Compare(str1, str2, true) == 0

Don’t check for null strings like this.

  if (str1 == null || str1 == "")

Use string.IsNullOrEmpty.

  if (string.IsNullOrEmpty(str1))

If you need to format your strings in a certain way, use string.Format

  string result = string.Format("Adding {0} to {1} equals to {2}.",  5, 6, 11);

Last of all, if you have to perform a lot of string manipulation, use a StringBuilder instead. It’s faster, more efficient and does not allocate new memory everytime it’s contents are changed.

Override Equals and GetHashCode

I have often see code like this, trying to check if a certain object (reference types) exists in a list. In this example, the unique identifier for the Person class is Id property.

public bool DoesPersonExist(List
<person> people, Person person)
{
    foreach (Person p in people)
    {
        if (p.Id == person.Id)
            return true;
    }
    return false;
}

In this case, you cannot use List.Contains method because it will be checking by reference to the same location in memory. Instead, one should override the Equals and GetHashCode methods. The Person class will look like this.

public class Person
{
    public Person(int id)
    {
        this.Id = id;
    }

    public int Id { get; private set; }

    public override bool Equals(object obj)
    {
        if (obj != null && obj is Person)
        {
            Person person = (Person)obj;
            return this.Id.Equals(person.Id);
        }
        return false;
    }

    public override int GetHashCode()
    {
        return this.Id.GetHashCode();
    }
}

And now you can just simply invoke

  return people.Contains(person)

Deep Copy

In my early days programming in C++, I learnt the importance of Deep Copy vs Shallow Copy. Shallow copy works relatively fine with value types, but not for reference types at all. What many people don’t get is that a simple assignment like this

  Person a = new Person();
  Person b = a;

actually means that the pointer of b is pointing to the memory location where a is pointing to. When you modify a, b is also changed. I’ve seen disastrous things happen when programmers ignorantly assign objects by reference which result in adverse effects. Shallow copy does not solve this problem, and you need to perform a deep copy. I used an Extension Method so that all reference types can call the DeepCopy method.

  using System.IO;
  using System.Runtime.Serialization.Formatters.Binary;

  public static class Extensions
  {
      public static T DeepCopy<T>(this T obj) where T : class
      {
          MemoryStream ms = new MemoryStream();
          BinaryFormatter bf = new BinaryFormatter();
          bf.Serialize(ms, obj);
          ms.Position = 0;
          T copy = bf.Deserialize(ms) as T;
          ms.Close();
          return copy;
      }
  }
  // calling DeepCopy
  Person p = new Person();
  Person n = p.DeepCopy();

Methods vs Comments

I’m a believer in writing compact, readable methods with good descriptive names rather than writing tons of comments in code. My argument is that someone else should be able to understand the logic just by reading your code. Comments should only be necessary if your code is performing something extremely complex or adhering to a specific business rule that someone else will have trouble understanding. Another place where I find comments are necessary is at the class scope, with a short summary of the responsiblity of the class. Again this is a personal programming habit, others may beg to differ.

Define your team’s coding standard and rules

Coding standards and rules may differ slightly from one project team to another. Before a reviewer can comment on your code, the project team should sit down, discuss and finalize upon an agreed set of standards and rules. Otherwise reviews will end up becoming a reviewer enforcing his/her own personal standards on you, which again will differ with the next reviewer.

Conclusion

Code review is an exremely important aspect of a software project, and should be encouraged. Code reviews can be daunting to most people, since no one likes to be told their code sucks. In order for this process to be pleasant for both parties, reviewees should be prepared to explain what their code does, and justify their decisions reasonably; reviewers should always criticize constructively, and not judge harshly or with arrogance. Remember, both parties stand to benefit from the review experience, code quality is assured…the way I see it, it’s a win win situation. 🙂

Share this post :
Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: