7
\$\begingroup\$

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)
    );
  }
}
\$\endgroup\$
3
  • \$\begingroup\$ @Mast so how would you proceed if you needed to write it as a production level code? should there be a distinction between a code for review here and a production level code? Are we here to review just the toy code? \$\endgroup\$ – CodeYogi Apr 24 '16 at 18:52
  • \$\begingroup\$ Funny you should ask, I put something similar written in Ruby up for review not too long ago. \$\endgroup\$ – Mast Apr 24 '16 at 19:19
  • \$\begingroup\$ @Mast its because the exercism team took the reference from that site. \$\endgroup\$ – CodeYogi Apr 24 '16 at 19:22
5
\$\begingroup\$

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")

\$\endgroup\$
8
  • \$\begingroup\$ exposing nucleotideCounts was the part of the exercise to pass the tests. Although your recommendation seems valuable to me. \$\endgroup\$ – CodeYogi Apr 24 '16 at 19:06
  • \$\begingroup\$ regarding the third point, I need to put that check in my constructor. Something, with a method like checkPrecondtion or validateArguments right? \$\endgroup\$ – CodeYogi Apr 24 '16 at 19:13
  • \$\begingroup\$ you could have a validateNucleotide(char) method, that throws if invalid. You could call this method from count and also from the loop in countNucleotides \$\endgroup\$ – janos Apr 24 '16 at 19:15
  • 2
    \$\begingroup\$ Building the data structure is the purpose of this class. So it's a well justified exception to the general rule to avoid computation in the constructor. \$\endgroup\$ – janos Apr 24 '16 at 19:21
  • 1
    \$\begingroup\$ That's what I tried to explained in "If you really need that method, then you cannot remove it". So in your case, you're not breaking encapsulation: you're simply fulfilling your client requirements by providing that method. \$\endgroup\$ – janos Apr 25 '16 at 5:12
4
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Note that containsKey('A') on the Map will currently always return true, as ACGT are all initialized to 0 in the map. \$\endgroup\$ – Simon Forsberg Apr 25 '16 at 16:48
2
\$\begingroup\$

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.

\$\endgroup\$
7
  • \$\begingroup\$ Hmm, I see, It would also let me write more domain specific code, right? \$\endgroup\$ – CodeYogi Apr 25 '16 at 17:45
  • \$\begingroup\$ What would happen if the client needs to call count('A')? where and how this conversion from char to enum takes place? If I am not asking too much, can you please put an example, please? \$\endgroup\$ – CodeYogi Apr 25 '16 at 17:56
  • \$\begingroup\$ I think this would help me stackoverflow.com/a/604426/4260745 \$\endgroup\$ – CodeYogi Apr 25 '16 at 18:17
  • \$\begingroup\$ @CodeYogi an Enum is still a class, you can add a static method to convert a char to your enum. See this answer stackoverflow.com/a/2965252/2115680 ( you posted your comment at the same time) \$\endgroup\$ – Marc-Andre Apr 25 '16 at 18:17
  • 1
    \$\begingroup\$ @CodeYogi Yeah, that looks quite fine to me \$\endgroup\$ – Simon Forsberg Apr 26 '16 at 14:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.