~pperalta

Thoughts on software development and other stuff

Code Review

with 3 comments

Yesterday I had the chief Coherence architect review some of my code before submitting it to Perforce. (No it wasn’t done by Cameron.) I actually look forward to them because I always learn something new. This time what stuck with me was the refactoring of a method and the before/after difference.

Here’s the before:

protected void configureAffinitySuffix()
    {
    String sJvmRoute = m_sConfiguredJvmRoute;
 
    if (sJvmRoute != null)
        {
        CacheFactory.log("jvmRoute set to '" + sJvmRoute + "' via configuration", 6);
        }
    else
        {
        sJvmRoute = getJvmRouteViaJmx();
        if (sJvmRoute == null)
            {
            m_sAffinitySuffix = "";
            CacheFactory.log("jvmRoute is not configured", 6);
            }
        }
 
    if (sJvmRoute != null)
        {
        CacheFactory.log("Configured affinity suffix: " + sJvmRoute, 4);
        m_sAffinitySuffix = SUFFIX_SEPARATOR + sJvmRoute;
        }
    }

Including blank lines, this is a 24 line method; and it isn’t very complex at all. Here is the newly refactored version:

protected void configureAffinitySuffix()
    {
    String sJvmRoute = m_sConfiguredJvmRoute;
    if (sJvmRoute == null)
        {
        sJvmRoute = getJvmRouteViaJmx();
        }
 
    if (sJvmRoute == null)
        {
        m_sAffinitySuffix = "";
        CacheFactory.log("The jvmRoute setting is not configured", 3);
        }
    else
        {
        m_sAffinitySuffix = SUFFIX_SEPARATOR + sJvmRoute;
        CacheFactory.log("Configured affinity suffix: " + sJvmRoute, 3);
        }
    }

The main differences are:

  • The new method is 19 lines, reduced from 24
  • All of the if statements are consistent; they’re all testing for a positive comparison
  • There are no nested blocks
  • The logging is more consistent, both in the ordering (the logging appears after the relevant line of code) and in the log level. There is also less logging so as to prevent the logs from filling up with noise.

All in all, we didn’t change the functionality of this method at all; however it is much cleaner and easier to follow. This is the attention to detail that IMHO is a significant factor in the quality of Coherence. (BTW, all submissions are peer reviewed before checking into source control.)

Written by Patrick Peralta

April 22nd, 2009 at 6:33 pm

Posted in Development

«
»

3 Responses to 'Code Review'

Subscribe to comments with RSS or TrackBack to 'Code Review'.

  1. It is much cleaner to read. Is the opening bracket on a separate line used standard for that purpose as well?

    Is peer review facilitated by a tool like Crucible or are they done using meetings and process?

    Matt Trefethen

    22 Apr 09 at 8:21 pm

  2. That’s what refactoring is for. Improve the code but keep the functionality.
    Anyway, your project should consider a decent Java Coding Convention. Like all strongly typed languages, Java doesn’t need the C-like Hungarian Notation in every variable name. Just ask yourself, what will happen if you have to change the type of the variable? Answer: You have to change all your dependent code. I’m just to lazy such stuff…

    Cheers,
    –olaf

    Olaf Heimburger

    23 Apr 09 at 1:08 am

  3. We don’t use Crucible, but as huge Atlassian fanbois, we should consider taking a look, especially for remote code reviews. For the most part it is done peer to peer, but sometimes for new developers or for large submissions, we’ll throw it up on a projector in a conference room.

    The coding style is a dialect of Whitesmiths (http://en.wikipedia.org/wiki/Indent_style). It took some getting used to from the K&R variant that most Java developers use, but I’ve actually grown quite fond of it.

    The Hungarian-esque convention threw me off a bit too but I’ve grown to appreciate it as well. Although Java is a strongly typed language, you don’t always have the type declaration sitting in front of you, so it’s nice to be able to distinguish at a glance the variable type (and whether it is a member or local variable.)

    We use Intellij IDEA, so changing the name of a variable is easier than changing the font on a Word doc. ;)

    Patrick Peralta

    23 Apr 09 at 6:45 am

Leave a Reply