Category: Programming

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.

Java Generics fun

I have played with Java generics on various projects and used them but I found the following chunk of code very useful for making me fully understand the possibilities:

    public  TResponse handleRequest(
            DataExtractRequestHandler requestHandler, Class responseType,
            TRequest request, 
            String uniqueId) {
        long startTime = new Date().getTime();
        String methodName = request.getClass().getSimpleName();
        logMethodEntry(methodName, uniqueId);
        TResponse response;
        try {
            response = requestHandler.handle(request);
            setResponseStatus(response, WrapServiceResponseStatus.SUCCESSFUL_REQUEST);
        } catch (WrapServiceException e) {
            response = buildErrorResponse(responseType, e.getErrorCode(), e.getDescription());
        } catch (Throwable e) {
            getLog().error(logPrefix(methodName, uniqueId) + " Unhandled Exception", e);
            response = buildErrorResponse(responseType, WrapServiceResponseStatus.UNHANDLED_EXCEPTION.getCode(), "Unhandled Exception: " + e);
        logMethodExit(response, request.getClass().getSimpleName(), request.getClass().getSimpleName(), uniqueId, startTime);
        return response;

In this code, we pass in a request handler that itself uses the generics interface to identify the specific class of the request and response type that the handler will user. The request handler implements the following interface:

    public interface DataExtractRequestHandler {
        TResponse handle (TRequest request) throws Throwable;

So we have used the handleRequest method in the first code block to provide a standard way of running the handle method of each of the different type of request handlers that implement the interface in the second block. In that way, we don’t have to duplicated all the logging calls in each request handler and we have a single place to do all the status handling (i.e. setting success on the response or dealing with any exceptions thrown by the handle method).

The primary driver behind this implementation was that when each request handler was responsible for dealing with its own exceptions and creating the appropriate response, there were some that missed a few of the exceptions thrown and therefore would return an exception via the webservice rather than a response that the consumer could handle.