Job Hunting – Code Reviews

At my current company, I am sent at least one code submission to review each week. I am a pretty lenient reviewer – show me some decent code and I’ll get you in for an interview so we can see what you are all about in person. That said, I am getting less and less patient with some of the things that I have seen so I thought I would put together a blog post about the issues to try and correct the problems before they land up on my desk. These are simple things that should take a normal developer about 5 minutes to think about and fix.

  1. Input guards – Most applications that are created for code reviews, including all 3 of the options we give to our potential recruits, use some form of input. The method of input is immaterial to the program but one thing that shouldn’t be forgotten is that you need to provide some verification that the input conforms to whatever is expected. Implementing this is simple – Regex and boundary checks should validate any possible inputs – but without it, I can easily find ways to break the implementation you have delivered and I am more than likely to pass on your code.
  2. Unit tests – Is there any developer out there who doesn’t yet know about unit tests? You don’t have to have 100 percent coverage – I’m not looking to see how you tested that a constructor worked – but you should have coverage around the major pieces of business logic in your code. If you don’t (and you are not coming to us straight out of uni), your code goes straight into the bin.
  3. Comments – Comments are only useful in one situation – where it isn’t obvious why things have been done in a specific way. Even here I would rather refactor the code to take away the ambiguity but in some cases, notably where the code you are writing relies on a large amount of legacy code, you are forced to do things in a certain way because of the framework you are working within. Nothing though is worse than comments that only reiterate what the method names already tell me. If the method is called determineTotalCost() and there is a comment such as Determines the total cost of the object, this is a total FAIL.
  4. Unused variables/fields/methods – Just because your IDE provides a way to automatically create both GET and SET methods, doesn’t mean you should. If you have no reason to use these methods (i.e. you use constructor injection and only use the values internally), then don’t put them in. Methods that do nothing simply complicate a code base. If it is a legacy method that you are worried someone might want to use later, rely on your Version Control System to provide them with the ability to resurrect it – don’t just keep it around in case. Same goes for variables and fields – if you added them at some point for a specific reason but later changed the code, make sure you go back and clean up. These unused pieces of code are more annoying because IDEs provide a way to identify unused code – find this option and make it an ERROR instead of just a WARNING.
  5. Not including all external libraries required to compile – Finally, when you submit your code, please create a package that includes EVERYTHING I will need to run it. Don’t assume that I have that library you have used in your code (i.e. junit). If you have a lib directory for your code, give it to me as well so I don’t have to worry about downloading diverse jars just to get your code to compile and run on my machine. If you think that the required library/technology is too large (ANT for instance), then at least provide the info in a ReadMe with a link to a download page so I don’t have to go hunting.

What I really want to impart with this post is that when you are submitting code to a company for them to assess, you want to make sure that it is SIMPLE. A simple implementation that solves the problem with a simple means for the assessor to open and run the code. A great code submission is not overcomplicated to show off a recruit’s skills – it is a simple implementation that gets the job done and doesn’t aggravate the assessor by forcing them to jump through hoops to get it to run.