Refactoring code

These are random notes taken while refactoring code at work

Patterns, Anti-patterns, best and worth practices, etc.

Don’t try too hard with your names (anti-pattern)

When define name for your entities one should be concern with what the entity is rather than how it relates to the different entities. Good example would be a “TestRunResultResultTest” which gives not a slightest hint that this is a POJO (plain old java object) that holds data produced by validation logic applied to the execution of the single shell script. Name like “ValidationResult” would describe this entity much better and don’t produce monsters such as testRunResultResultTestDao.getTestRunResultResultTestForTestRunResult (say it aloud 5 times)

Don’t add features until asked by customer

I’m referring to the validation above which seems to do the same job as test script only it is harder to configure. The following is email that I send out that explains my point of view

AIMV back end that I’m refactoring now has a “Validation” part. In the back end code it’s not really implemented (the only validation there is some hardcoded regexpression applied to stderr output). From what I understand validation is supposed to validate the output produced by the script and should be driven by the database table that holds things like validation type, expected outcome, etc.

Since we can have multiple scripts attached to each test case I have a hard time distinguishing between the validation and having another script in the test case if the existing one(s) does not provide sufficient information. I think for the most cases just the outcome of the individual shell script (exit code) should be sufficient to indicate success of failure. If such script produces the output that needs to be parsed additionally it would be easier for the script writer to add another script and to pipe and parse the outcome. I propose to remove post-validation from the database and the code and instead rely on ability to add multiple scripts to the individual test case. To give user chance to determine “what went wrong” we will provide ability to look at the produced output (stdout and/or stderr) from the front end for each executed script

Unnecessary exposure to external code

Yes, yes – we do use Spring and probably always will. However that doesn’t mean we have to put direct references to the framework classes into our code. Good example is our DAO interfaces where each method throws org.springframework.dao.DataAccessException. There’s actually another reason why it’s wrong: best practice calls for you defining your own exceptions by extending common Java API exceptions. So in the case of DAO method the appropriate way of declaring the DAO exception would be extending java.lang.RuntimeException and choosing appropriate name (like AimvDataAccessException). By doing this our interfaces will stay framework neutral which they should as much as possible

Non-functional screen elements

If you have screen elements that are not functional – it’s probably a good idea to remove or hide these unless you are actively working on it

Never nest your loops

If you have one FOR or WHILE loop nested inside another one it’s time to refactor the internal loop into it’s own method. It dramatically improves readability of the code and promotes the reuse (how many times did you cut/paste your loop today?)

Replace complex logic inside the IF test (not body) with a method

So if you have logic inside your IF parenthesis spilling to the second line its time to grab that code and put in into a local variable or even better – refactor it to a method that returns a boolean. It’s very simple if you are using Eclipse. Just highlight the code and do right-click->Refactor->Exctract Method. So

if (dis == that && (that != this.somethingElse || foo.equals(this.boo)) {
    // do something
}

becomes

if (isThisSane(dis, that, foo)) {
    // do something
}

Copy/Paste as a root of all evil

If you finding yourself copying and pasting parts of your code most likely you are committing a sin-of-sins. Actually there are tools that written just for the purpose of identifying this anti-pattern. Try PMD plugin on your code and you will be very surprised of results. And why not to do it? Copy/paste promotes code duplication which leads to linear, procedural, spaghetti-like and hard to maintain code. Before you copy/paste see if you can extract reusable method or even class and then it’s perfectly fine to reuse that.

Why coding standards are important?

If the standards were followed then we wouldn’t have Script_Packages.java and ScriptPackage.java in the same folder.

Don’t make your models too special

Models ahould be POJOs with no 3rd party dependencies. For example, we have Script domain object that extends Package. That one in turn implements JSONSerializable interface. As result – backend application has to include json.jar as one of dependencies

DO NOT (Sorry I’m yelling) use System.out.println

Why? I’ll give you 3 reasons

• You are not using debugger, which means you are operating at 20% of your potential productivity
• You are not using logging facilities (Java logging or Log4j) which in a turn means that you are polluting your code left and right and the only choice you have is to remove sysouts manually or leave these in code which will slow your code down
• It’s not cool
  Generally sysouts should not be substitutes for tool-based debugging, and you should have carefully though through logging statements in strategic places that should be permanent part of your code. Verbosity of your log should not be regulated by cut and paste but rather by setting debug level in configuration file

Do not swallow your exception

The exception handling will be eventually put in the separate article but most importantly follow these simple rules:

• Rethrow (or wrap into RuntimeException) your exceptions and let them “bubble up”
• If catching and rethrowing (wrapping) do not forget to pass original exception as an argument (or you will loose stacktrace and messages)
• When you finally deal with the exception, inside your CATCH block use LOG.error(Exception) or at very least e.printStackTrace(). If you don’t, your log will never reflect the valuable information