Code Criticism

I’ve been working on a project using the Google Data API.  Given my preferences, I’d be coding in Perl.  Java is the least evil of all the languages that GData supports.  I’d like to do a little self-critique of my coding style in Java since It’s imported from C.

Here‘s the source file I’m critiquing.

  • 32-36:  My TODOs used to be here.
    • /*
  • 38-40:  A comment on the GNU-centricity of the CLI.
    • /*
      * This is very GNU-centric code.  It’s portable but will still behave as if it were a GNU command.
  • 64-67:  Telling everyone where I got the regex to validate e-mail addresses.
  • 151:  It’s the MySQL Connector/J driver.
    • // MySQL Connector/J driver
  • 153-156:  Code that’s redundant.  And also broken.
    • // this code is redundant (and also broken)
      // if (dbHost.isEmpty() || dbHost == null) {
      // dbHost = “localhost”;
      // }
    • It’s redundant because the defaulting is handled in the main method.
    • It’s broken because if dbHost really is null, then the first test in the if() statement would throw a NullPointerException.  I would need to reverse the tests to have the null check first.  Thanks to Java’s short-circuit conditional parser, it would return true from the ==null test and not even check for string emptiness.
  • 215:  There really is no reasonable default for a database username.
    • // no reasonable default
  • 216:  If we default to null, then MySQL assumes that we have no password on the account and doesn’t prompt for one.  If we default to an empty string, MySQL will prompt for a password.  Not that we want that in this case but maybe if I ever make a GUI for it.
    • // allows for empty password
  • 221:  Explains the code for setting the default last_modified date.
    • // this sets our default last modified date to be yesterday
  • 227:  Explains the code to handle command line arguments.
    • // command line argument parsing
  • 229:  Mostly for me because I’m used to the C implementation of GNU getopt().
    • // the character representing the option the user chose
  • 231-234:  A comment on my lack of typing ability.
    • //
      // StringBuffer sb = new StringBuffer(); // note – not StringBugger
      // like
      // I keep wanting to type!
    • If this were C or C++, I would just do #define StringBugger StringBuffer.
  • 251-253:  A Time to Complain.
    • // complain to the user when a REQUIRED_ARGUMENT
      // isn’t present
  • 257:  A case that can’t happen.
    • // can’t happen
  • 262-263:  Specifies what to do if someone gives us data without telling us what the data is for.
    • // someone tried to give us data without an option – doesn’t
      // work that way
  • 302-303:  The default case according to GetOpt.
    • // Getopt didn’t find the option in the list of
      // choices we gave it
  • 308-310:  The real default case.
    • // shouldn’t happen
    • throw new UnhandledCaseException(
      “The programmer screwed up the command-line argument handling.  This is a bug.”);
    • I could have just stacked the case ‘?’: and default: labels so that they both executed the same code.  But I wanted to do the Right Thing and isolate user error from programmer error.
  • 315-323:  Explains what happens when the user tries to be too clever or this program is piped output from another text-generating source.
    • // This shouldn’t be able to happen – getopt() returns 1 when
      // there’s an option argument given with no option specified.
      // But if the user terminates the options list with a — and
      // then puts stuff, getopt() will return -1 and break the loop
      // and this code will handle the arguments after the –.
    • System.out
      .println(“I don’t know what ”
      + args[i]
      + ” means so I’m ignoring it.  I hope that’s okay.  If not, CTRL+C is your friend right now.”);
  • 339-342:  I caught this bug in Unit Testing.  Short circuit evaluation is nice when you actually use it.
    • if (dbUser == null || dbPass == null || dbUser.isEmpty()) {
    • // I had to fix this to put the null checks first in case dbUser
      // really is null. Calling isEmpty() would cause a
      // NullPointerException.
  • 358-359:  A note on a possibly-outdated Google Calendar URL.
    • // This might be an old URL. If it fails with an HTTP exception,
      // try changing this.
  • 360-361:  A TODO.
    • // TODO look up how to post events to a calendar other than the
      // user’s private calendar.
  • 431:  A comment to help me sort out the ConnectionBucket handling code.
    • // We found a bucket that matches this event, so add it.
  • 437-438:  See 431.
    • // We did not find a bucket to match the event, so make one and
      // then add the event to it.
  • 444-448:  An explanation of the rationale behind validating e-mail addresses on the client side.
    • // validate username as e-mail address
      // The rationale is that we want all our ducks in a row when we
      // connecto to Google
      // and try to authenticate. My server’s CPU cycles are a lot
      // cheaper than Google’s.
  • 450-454:  A philosophical internal argument over when it’s appropriate to throw an exception.
    • // I originally threw a ValidationException here but after
      // reviewing the Java manuals, I determined that failing an
      // e-mail validation is not considered an exceptional
      // situation and opted for normal program flow controls
      // instead.
  • 458-459:  Sort of a sad thing to note that we did all that prep work and now we’re just going to let it get garbage collected.
    • // Just don’t make this bucket and discard any
      // events that would be put into it.

I tried to get just the comments since even non-programmers should understand the reasoning that went into them.  I welcome public comments on my coding style and programming mindset.  I do know that I tend toward procedural programming since I normally do C/UNIX systems programming.

  1. #1 by Joshua on May 12, 2010 - 2:30 PM

    Lol. I went to run the JUnit tests for another class (ConnectionBucket) and I had forgotten to comment out the default calls to fail() which were the first lines of every test. That was the most useless JUnit test ever written.

  2. #2 by Joshua on May 12, 2010 - 2:48 PM

    Woohoo!!! ConnectionBucket passes JUnit! I’ll celebrate with another Bawls G33K B33R!

  3. #3 by Joshua on May 12, 2010 - 4:17 PM

    Event also passes. Yayy!! Pizza for me!

Leave a Reply

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

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

Google+ photo

You are commenting using your Google+ 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 )


Connecting to %s

%d bloggers like this: