Nucleotide Count

Question

I am learning Java and hence practicing it from here. The solution seems pretty trivial to me but still I would like to have a honest review which would guide me to following ideas:

1. Am I maintaining class Invariants?
2. Do I need to have more Domain specific error messages?
3. Although the solution seems trivial but I am missing some performance boost?
4. Am I following proper encapsulation?
5. Using correct data structures?
6. Any other suggestion are most welcome.

import java.util.Map;
import java.util.HashMap;
import java.util.Collections;

public class DNA {
  private final Map<Character, Integer> nucleotideMap = new HashMap<>();
  private final String dnaSequence;

  public DNA(String dnaSequence) {
    this.dnaSequence = dnaSequence;
    initializeDefaultNucleotideMap();
    if (!dnaSequence.isEmpty()) {
      countNucleotides();
    }
  }

  private Map<Character, Integer> initializeDefaultNucleotideMap() {
    nucleotideMap.put('A', 0);
    nucleotideMap.put('C', 0);
    nucleotideMap.put('G', 0);
    nucleotideMap.put('T', 0);
    return nucleotideMap;
  }

  private void countNucleotides() {
    for (Character chr : dnaSequence.toCharArray()) {
      nucleotideMap.put(chr, nucleotideMap.get(chr) + 1);
    }
  }

  public int count(char nucleotide) {
    if (!nucleotideMap.containsKey(nucleotide)) {
      throw new IllegalArgumentException("Invalid nucleotide");
    }
    return nucleotideMap.get(nucleotide);
  }

  public Map<Character, Integer> nucleotideCounts() {
    return Collections.unmodifiableMap(nucleotideMap);
  }
}

Test suites:

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.entry;

import org.junit.Test;

public class NucleotideTest {
  @Test
  public void testEmptyDnaStringHasNoAdenosine() {
    DNA dna = new DNA("");
    assertThat(dna.count('A')).isEqualTo(0);
  }

  @Test
  public void testEmptyDnaStringHasNoNucleotides() {
    DNA dna = new DNA("");
    assertThat(dna.nucleotideCounts()).hasSize(4).contains(
        entry('A', 0),
        entry('C', 0),
        entry('G', 0),
        entry('T', 0)
    );
  }

  @Test
  public void testRepetitiveCytidineGetsCounted() {
    DNA dna = new DNA("CCCCC");
    assertThat(dna.count('C')).isEqualTo(5);
  }

  @Test
  public void testRepetitiveSequenceWithOnlyGuanosine() {
    DNA dna = new DNA("GGGGGGGG");
    assertThat(dna.nucleotideCounts()).hasSize(4).contains(
        entry('A', 0),
        entry('C', 0),
        entry('G', 8),
        entry('T', 0)
    );
  }

  @Test
  public void testCountsOnlyThymidine() {
    DNA dna = new DNA("GGGGGTAACCCGG");
    assertThat(dna.count('T')).isEqualTo(1);
  }

  @Test
  public void testCountsANucleotideOnlyOnce() {
    DNA dna = new DNA("CGATTGGG");
    dna.count('T');
    assertThat(dna.count('T')).isEqualTo(2);
  }

  @Test
  public void testDnaCountsDoNotChangeAfterCountingAdenosine() {
    DNA dna = new DNA("GATTACA");
    dna.count('A');
    assertThat(dna.nucleotideCounts()).hasSize(4).contains(
        entry('A', 3),
        entry('C', 1),
        entry('G', 1),
        entry('T', 2)
    );
  }

  @Test(expected = IllegalArgumentException.class)
  public void testValidatesNucleotides() {
    DNA dna = new DNA("GACT");
    dna.count('X');
  }

  @Test
  public void testCountsAllNucleotides() {
    String s = "AGCTTTTCATTCTGACTGCAACGGGCAATATGTCTCTGTGTGGATTAAAAAAAGAGTGTCTGATAGCAGC";
    DNA dna = new DNA(s);
    assertThat(dna.nucleotideCounts()).hasSize(4).contains(
        entry('A', 20),
        entry('C', 12),
        entry('G', 17),
        entry('T', 21)
    );
  }
}

Answer 1

Am I maintaining class Invariants?

Yes. But I wonder if you really need to expose nucleotideCounts. You did it safely, by wrapping the map in unmodifiableMap (and because the values are immutable too). To eliminate the risk of abuses and errors, it would be better to keep this implementation detail hidden.

Do I need to have more Domain specific error messages?

I don't see an obvious reason to do that. The IllegalArgumentException seems appropriate when trying to get an invalid nucleotide.

Although the solution seems trivial but I am missing some performance boost?

Since the number of nucleotides is unlikely to change, using a Map for the storage seems a bit overkill. A simple array of 4 elements would be more light-weight. That being said, premature optimization is considered evil, so I wouldn't worry about this too much until the current implementation is proven to be a bottleneck.

Am I following proper encapsulation?

As mentioned earlier, it would be better to hide the implementation detail of the storage of the nucleotide counts, that is, remove the nucleotideCounts method. But it depends on your use case. If you really need that method, then you cannot remove it. In any case, it's important to ask that question: should this be hidden?

Using correct data structures?

Yes.

Any other suggestion are most welcome.

Some additional tips and observations:

1. No need to keep dnaSequence in a field. Once you built the map of counts from it, you no longer need it. I suggest to get rid of it.

2. In the constructor, it's unnecessary to check if dnaSequence is empty. The iteration logic in countNucleotides naturally embeds that check.

3. You'll get an NPE on invalid nucleotides, for example new DNA("hello")

Answer 2

Don't expose too much

You currently have a method nucleotideCounts that returns the map of each nucleotide with its count in the DNA sequence. But do you really need this method? It is very correctly implemented by returning an unmodifiable map, but I would argue that you should remove the method completely.

So far, your DNA class is keeping track of the count of nucleotide through a map. But this is internal information / an implementation detail. A client of DNA does not need to know how the count is being maintained. The logic of operating with a DNA sequence should go through the DNA class itself, because it is responsible for that. This is the goal of object-oriented programming: having an object responsible for a specific task.

As an example, currently, if a client needs to know if a DNA contains a specific nucleotide, they need to do:

boolean hasA = dna.nucleotideCounts().containsKey('A');

Instead, consider creating a method hasNucleotide inside the class DNA:

public boolean hasNucleotide(char nucleotide) {
    return nucleotideMap.containsKey(nucleotide);
}

Then a client can call boolean hasA = dna.hasNucleotide('a'); without getting a reference to the internal data structure being used.

Of course, implementing this now requires you, the DNA class maintainer, to know what kind of methods you will want to add and support (at least as a start, you can of course add methods later).

In my view, this is the most important comment.

Remove redundant checks

In your constructor, you are doing

if (!dnaSequence.isEmpty()) {
    countNucleotides();
}

but the if is not really needed since in the case of an empty DNA sequence, nothing will happen inside the countNucleotides() method. Might as well simplify the code and remove the check.

Beware of null

You are not checking whether the given dnaSequence in the constructor of DNA is null. With your current code, if you give null then it will fail with a NullPointerException.

This could be a wanted behaviour. Some JDK classes also follow this convention. However, if this is indeed wanted then it is best to document it properly in the constructor's Javadoc.

Worth mentioning, with Java 7, you could use requireNonNull(obj, message) to throw an exception if the given object is null with the given message.

Answer 3

Enum, an enum, my kingdom for an enum! an answer about an enum!

As far as I know, there are always four different nucleotide types. A, C, G, T. What do programmers do when there is always a limited amount of something? They make an enum.

public enum NucleotideType {
    A, C, G, T;
}

By using this little type as a key in your map, so that you have Map<NucleotideType, Integer>, you can handle your class in better ways. (See the tips provided by Tunaki and Janos).

In fact, even better would be to use an EnumMap<NucleotideType, Integer> which would likely offer a performance-boost.