4
\$\begingroup\$

The code counts the amount of unique characters that fit in the character set ACGT (stemming from nucleobases).

We take a string as input that must contain only either 'A', 'C', 'G', or 'T'.

For instance, "AACCC" should return the Map that looks like this:
Map('A' -> 2, 'C' -> 3, 'G' -> 0, 'T' -> 0).
More detail can be found in a gist with specs.

class DNA(code: String) {
  val nucleotides = Map('A' -> 0, 'T' -> 0, 'C' -> 0, 'G' -> 0)

  code foreach valid_nucleotide

  def nucleotideCounts(): Map[Char, Int] = {
    nucleotides ++ code.filter(nucleotides.contains(_)).groupBy(identity).mapValues(_.length)
  }

  def count(nucleotide: Char): Int = {
    valid_nucleotide(nucleotide)
    nucleotideCounts()(nucleotide)
  }

  private def valid_nucleotide(char: Char) = {
    if (!nucleotides.contains(char)) {
      throw new IllegalArgumentException("DNA is wrong");
    }
  }

}
\$\endgroup\$
  • \$\begingroup\$ So this is only for DNA? What if you wanted to support RNA? \$\endgroup\$ – Vogel612 Jul 1 '15 at 10:06
  • \$\begingroup\$ idk, it was just a test task for learning purposes. I have no idea about RNA. What difficulties am I going to have when I try to add the RNA support in my code? \$\endgroup\$ – HeeL Jul 1 '15 at 14:10
  • \$\begingroup\$ well... the allowed characters are different :) \$\endgroup\$ – Vogel612 Jul 1 '15 at 14:11
  • \$\begingroup\$ then I will have to only modify a Map \$\endgroup\$ – HeeL Jul 1 '15 at 14:12
  • \$\begingroup\$ I would suggest modfying the returned argument "DNA is wrong" to: "Character not found in valid nucleotide set". \$\endgroup\$ – Quill Jul 1 '15 at 14:28
3
\$\begingroup\$

For the most part, there isn't much that I would change - well done!

First, minor things:

  1. Remove the semi-colon in valid_nucleotide.

  2. Next, you can change nucleotides.contains(_) (an anonymous function) to just nucleotides.contains (a method value).

  3. The reuse of valid_nucleotide in count will result in any call with an invalid parameter getting an IllegalArgumentException("DNA is wrong"). I think this error would be confusing in this context.

The biggest single change I would make is to precompute the nucleotide counts. Currently, the code recomputes them each and every time either count or nucleotideCounts is called. Since code is immutable, it is only necessary to compute them once.

In a Java program, the "empty" nucleotides map could be made into a static member (rather than having a copy of it in every instance of the DNA class). The equivalent approach in Scala is to use a companion object (an object with the same class name as the class it is a companion to).

My version of your code looked like this:

object DNA {
  val nucleotides = Map('A' -> 0, 'T' -> 0, 'C' -> 0, 'G' -> 0)
}

class DNA(code: String) {

  code foreach validNucleotide

  val counts = countNucleotides()

  def nucleotideCounts(): Map[Char, Int] = counts

  def count(nucleotide: Char): Int = {
    validNucleotide(nucleotide)
    nucleotideCounts()(nucleotide)
  }

  private def countNucleotides(): Map[Char, Int] = {
    DNA.nucleotides ++ code.filter(DNA.nucleotides.contains).groupBy(identity).mapValues(_.length)
  }

  private def validNucleotide(char: Char) = {
    if (!DNA.nucleotides.contains(char)) {
      throw new IllegalArgumentException("Character not found in valid nucleotide set")
    }
  }
}
|improve this answer|||||
\$\endgroup\$

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.