Code Review, Five Style

Code review. That dreaded moment when a team gathers around for a series of soul-crushing criticism, indulging in fist fights even, over the legitimacy of their own ugly, but still their own, code.

Except it’s not like that at all.

Here at Five we take code review as part of our standard workflow. We have a simple rule of not putting any code into production without another pair of eyes being involved. Everyone gets its share of review, and everyone’s code gets reviewed – no exceptions.

Why?

Code review ensures developers speak the same language. Code, as the saying goes, is easier to write than read, so we make sure a level of comprehension is reached by all members of a team.

Secondly, code review saves time. You probably wouldn’t say it, given everything has to be approved, but, in the end, that code gets to be maintained, built upon or handled over to another developer. All easier when you’re dealing with peer-reviewed code.

Lastly, it’s a great tool for personal growth. Beside the obvious learning experience for the reviewed, a reviewer learns how to communicate technical problems effectively – valuable trait for any developer.

What we care about

As in human communication, we value clear, direct communication consisting of simple sentences that convey meaning, rather than obfuscate it through unnecessities or vague meanings.

Translated into programming languages, this just means good design. Clean code, clear APIs, OOP fundamentals got right – the weight of review is on design, rather than implementation. We may turn an eye on a piece code we know could be optimized, but we’ll raise hell if a function does too much, there’s unnecessary coupling, or simply a poor choice for a variable name.

Things we care also form a set of rules, a code standards we review and code by. This helps not only know what to look during a review, but also eliminate any far-fetched judgments. These rules are standard iOS / Android coding guidelines plus extra stuff we care about, mostly design. Single Responsibility Principle, Don’t Repeat Yourself and it’s alter-ego You Ain’t Gonna Need It are some of them.

How we do it

The process is pretty much enforced by tools we use, meaning it’s not even possible to close a task without reviewing it.

As for how it’s actually done, a reviewer will look at a particular diff or fire up IDE to look how the code fits its surrounding. The latter is preferred for all but trivial changes as your average pull request often misses context required for understanding the code.

Ok, sold!

Now, let`s look at the actual source code and do some code reviewing.

Example 1. An iOS library

Looking at class representing paywall. A paywall is something that limits access to content for users with subscriptions by asking them to login or purchase a subscription. Typically, we just call a method to check if user has access, and paywall does the rest.

@interface ZoopePaywallSDK : NSObject
+ (void)initSectionId:(NSNumber *)sectionId withNumFreeViews:(NSNumber *)numViews;
+ (void)numFreeArticlesInSection:(NSNumber *)sectionId;
+ (BOOL)isUserAuthorized;
+ (void)authenticateWithEmail:(NSString *)email
       andPassword:(NSString *)password
       withBlock:(void (^)(NSString *userId, BOOL valid, NSError *error))block;
@end

Wow, things had gone wrong here! First, ZoopePaywallSDK is a poor choice for a class name – SDK is unnecessary and Zoope, being the name of an organisation, is really the namespace. All the members are declared class level, even though they seem to apply to a particular paywall instance. Lastly, method names aren’t following up Cocoa naming conventions. authenticateWithEmail:andPassword:withBlock? Just do authenticateWithEmail:password:block – no reason for that extra and and with.

After a bit of refactoring, a new version comes out:

@interface ZOPaywall : NSObject
+ (id)initWithSectionId:(NSNumber *)sectionID numFreeViews:(NSInteger)numFreeViews;
@property (readonly) NSString *sectionID;
@property (readonly) NSInteger numFreeViews;
@property (readonly) BOOL isAuthorized;
- (void)authenticateWithEmail:(NSString *)email
                     password:(NSString *)password
                     block:(void (^)(NSString *userID, BOOL valid, NSError *error))block;
@end

One could argue whether authenticateWithEmail:password:block belongs here, but we’ll leave it there for now.

Example 2. an iOS app

Here’s a class used for tracking user invents. It’s being used through the app, so it kind of makes sense to have a single instance of it.

static Tracking *instance;
+ (void)initialize {
 instance = [Tracking alloc] initWithTrackingID:"UA-20547750-5"];
}
+ (instancetype)shared {
 return instance;
}

Couple of issues here – separate initialize and shared method to produce a singleton is impractical. A proper implementation of singleton is a single, static method (which, by the way, should be called sharedTracker) that initializes the object in a thread-safe way and returns it – also, hardcoding configuration values is ugly. That tracking ID belongs with the other configuration variables, perhaps a .plist or .pch file. Ideally, the app should be configurable without touching the source code.

Eventually, we rewrote this as:

+ (instancetype)sharedTracker {
static Tracker *sharedTracker = nil;
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
 sharedTracker = [[Tracker alloc] initWithTrackingID:TRACKING_ID];
});
return sharedTracker;
}

Example 3. an Android app

This is from an activity that handles user authentication, invoked once the user had entered user name and password and hit login.

private void login(final String pUsername, final String pPassword, final ProgressBar progress) {
  String url = String.format(URL_HOLDER, pUsername, pPassword);
  if (progress == null) {
    mAq.ajax(url, JSONObject.class, new AjaxCallback<JSONObject>() {
      public void callback(String url, JSONObject object, com.androidquery.callback.AjaxStatus status) {
        if (object.optBoolean("Valid")) {
          mPref.edit().putBoolean(LOGGED_IN_KEY, true).putString(USERNAME_KEY, pUsername).putString(PASSWORD_KEY, pPassword).commit();
        }
      };
    });
  }
  else {
    mAq.progress(progress).ajax(url, JSONObject.class, new AjaxCallback<JSONObject>() {
      public void callback(String url, JSONObject object, com.androidquery.callback.AjaxStatus status) {
        if (object.optBoolean("Valid")) {
          mPref.edit().putBoolean(LOGGED_IN_KEY, true).putString(USERNAME_KEY, pUsername).putString(PASSWORD_KEY, pPassword).commit();
          Toast.makeText(mContext, "Logged in...", Toast.LENGTH_SHORT).show();
          mLoginDialog.dismiss();
        }
        else {
          Toast.makeText(mContext, "Wrong credentials", Toast.LENGTH_SHORT).show();
        }
      };
    });
  }
}

A single method is doing calls to a server, parsing the JSON response, persisting user credentials and displaying the message – clearly a violation of a SRP. Obviously, the solution is to separate this kind of logic into components. For example, server logic would be handled by some an object that provides interface to the server API. Also, that Toast message belongs into strings file.

After some refactoring, here is what a new version looks like:

private void login(String username, String password) {
  server.login(new OnResponseListener() {
    @Override
    void onResponse(Response response) {
     if (response.status.isSuccess()) { // login success
        store.saveUsernameAndPassword(username, passsword);
        Toast.makeText(context, R.string.logged_success, Toast.LENGTH_SHORT).show();
        return;
      }
     // login failed
     Toast.makeText(context, R.string.logged_failed, Toast.LENGTH_SHORT).show();
    }
  });
}

Again, resulting in a simpler, easier to maintain version of what we had. Doing reviews and refactorings like this continually ensures it stays that way.