A Review of the work done in my genetic project

01.12.2014 - Lesezeit ca. 5 Min - 1000 Wörter

And I thought it would be clean and clear

The last weeks I have written some code about genetics in order to calculate the outcome of the combination of two alleles using TDD and I was quite happy with my code. Do not get me wrong, I’m still. That is why I have brought the code in the software craftsmanship community in my company in order to review the code. That was an experience!If you ever have the chance, do it. It’s definitely an interesting experience. So there I was, and presented the code. My colleagues are all good craftspeople. Professional developers. And some of them much better Java developers than I’ll ever be.

They gave me some insights in how Java really works. They also gave me many tips, tricks and show me possibilities to do things better.

Constants.

I have many constants in the tests, in order to avoid magic numbers. For instance I use ONE_ITEM_AVAILABLE to represent the number one. My colleagues’ opinion was this is unnecessary. I should use constants to mask numbers that has “an special meaning”. They meant that something like

1    assertEquals(TestConstants.ONE_ITEM_AVAILABLE, probabilities.getCount());

doesn’t help for better understanding. They meant:

It is easier to read it without the constant because I don't need to think about what the constant means to be. I'm asking about how many elements are contained in something, so the natural answer is to compare with a number.

It is for me the same thing, I don’t have any problems by translating ONE_ITEM_AVAILABLE to one but indeed I must translate it. What do you think about it? When should we use constants an when should we not use it?

Initializing the tests

As you can see I use a function to initialize every test.

1    @Before
2    public void InitializeTest() {
3        dominantHomozygousLocusA = AllelePairFactory
4                .createDominantHomozygous(TestConstants.LOCUS_A);
5        heterozygousLocusA = AllelePairFactory.createHeterozygous(TestConstants.LOCUS_A);
6        recessiveHomozygousLocusA = AllelePairFactory
7                .createRecessiveHomozygous(TestConstants.LOCUS_A);
8    }

This function initializes always three variables but in every test I need only two of them. I do so because I think that using it in this way the test implementation is shorter and clearer. My colleagues meant:

You are doing work that you don't need. Use separated functions to initialize only what you really need.

Well, thats right. I have thought about it and I mean it is better to initialize the values only once. The values are constant. So I only need to initialize it at the beginning. They don’t change. I can eliminate the @Before function and initialize them this way:

 1public final class AllelePairCombinationTest {
 2    
 3    ...
 4    
 5        private final static AllelePair dominantHomozygousLocusA = AllelePairFactory
 6                .createDominantHomozygous(TestConstants.LOCUS_A);;
 7        private final static AllelePair heterozygousLocusA = AllelePairFactory
 8                .createHeterozygous(TestConstants.LOCUS_A);
 9        private final static AllelePair recessiveHomozygousLocusA = AllelePairFactory
10                .createRecessiveHomozygous(TestConstants.LOCUS_A);;
11    
12    ...
13}

Test with expected exception

In my tests I’m checking for expected exception this way:

1    @Test(expected = IllegalArgumentException.class)
2    public void AddingAnAlreadyAddedItemThrowsIllegalArgumentException() {
3        results.add(String.class, Constants.FIFTY_PERCENT);
4        results.add(String.class, Constants.FIFTY_PERCENT);
5    }

My colleagues meant:

You should activate the exception when you expect it will be thrown, not before. You should do it in tests where you can get the expected exception more than once.

I didn’t know that’s possible, but that was indeed a good tip. These art of test can be written this way:

1    @Rule
2    public ExpectedException exception = ExpectedException.none();
3
4    @Test
5    public void AddingAnAlreadyAddedItemThrowsIllegalArgumentException() {
6        results.add(String.class, Constants.FIFTY_PERCENT);
7        exception.expect(IllegalArgumentException.class);
8        results.add(String.class, Constants.FIFTY_PERCENT);
9    }

Information text in asserts

I don’t use to write explanations in the assert commands. I hope the test can be understood by reading the name. Anyway, my colleagues meant:

As you read the result of a failing test, you see something like:

1java.lang.AssertionError: expected:<0.5> but was:<0.25>

This is not really clear and easy to understand. It will be better to assert with an informative text. So that the failing test result can be easily read:

1java.lang.AssertionError: A fifty percent probability was expected. 
2The actual probability was: 25.0 percent. expected:<0.5> but was:<0.25>

I will try to remember this.

There was also many other tips. But in my opinion, my colleagues found two bigger problems.

The first one was that I violated the DRY principle. I have implemented three different classes, which derived from AllelePairImpl. These classes implement the needed functionality to return the combinations with the other possible Alleles. It means, for instance, that the combinations between dominant homozygous and heterozygous are calculated in two different places. Once in the dominant homozygous class (combineWithHeterozygous function) and another time in the heterozygous class (combineWithDominantHomozygous function). And as I already say this violates the DRY principle.

The second one was something, I didn’t expect. I began this project because I want to practise Java and TDD. And by practising I forgot one of most important principles. Keep it simple stupid. KISS. The implementation is too complicated. The problem to be solved was really easy. There are three different possible allele pairs and they combine in fixed matters. An easy mapping between parent alleles and this fixed combinations will also do the job. I don’t really need all this classes and subclasses. It was good for me to practise, but this isn’t an operative solution. I will try to implement the problem in a way that accomplish with the KISS principle.

I would like to thank my colleagues for all the tips and insights that they have given me. In this way I hope to improve my skills.


If you enjoyed reading the article, I would be glad if you share it. :-)