5
\$\begingroup\$

This is my second day in learning Scala and I still need to develop a taste of functional programming, I often find myself doing imperative coding. Below is the result of my TDD practice.

Code

class DNA(dnaSequence: String) {
  var count = collection.mutable.Map('A' -> 0, 'T' -> 0, 'C' -> 0, 'G' -> 0)

  dnaSequence.foreach { (i: Char) =>
    validate(i)
    count = count.updated(i, count.get(i).getOrElse(0) + 1)
  }

  def nucleotideCounts() = count

  def nucleotideCounts(nucleotide: Char) = {
    validate(nucleotide)
    count.get(nucleotide).getOrElse(0)
  }

  private def validate(chr: Char) = {
    if (!count.contains(chr)) throw new RuntimeException()
  }
}

Test Suite

import org.scalatest._

class NucleotideCountSpecs extends FlatSpec with Matchers {
  "empty dna string" should "have no adenine" in {
    new DNA("").nucleotideCounts('A') should be (0)
  }

  it should "have no nucleotides" in {
    val expected = Map('A' -> 0, 'T' -> 0, 'C' -> 0, 'G' -> 0)
    new DNA("").nucleotideCounts should be (expected)
  }

  "a repetitive sequence" should "count cytosine" in {
    new DNA("CCCCC").nucleotideCounts('C') should be (5)
  }

  it should "have only guanine" in {
    val expected = Map('A' -> 0, 'T' -> 0, 'C' -> 0, 'G' -> 8)
    new DNA("GGGGGGGG").nucleotideCounts should be (expected)
  }

  "a mixed dna string" should "count only thymine" in {
    new DNA("GGGGGTAACCCGG").nucleotideCounts('T') should be (1)
  }

  it should "count a nucleotide only once" in {
    val dna = new DNA("CGATTGGG")
    dna.nucleotideCounts('T')
    dna.nucleotideCounts('T') should be (2)
  }

  it should "not change counts after counting adenine" in {
    val dna = new DNA("GATTACA")
    dna.nucleotideCounts('A')
    val expected = Map('A' -> 3, 'T' -> 2, 'C' -> 1, 'G' -> 1)
    dna.nucleotideCounts should be (expected)
  }

  it should "validate nucleotides" in {
    a [RuntimeException] should be thrownBy new DNA("GACT").nucleotideCounts('X')
  }

  it should "validate dna not rna" in {
    a [RuntimeException] should be thrownBy new DNA("ACGU")
  }

  it should "validate dna" in {
    a [RuntimeException] should be thrownBy new DNA("John")
  }

  it should "count all nucleotides" in {
    val s = "AGCTTTTCATTCTGACTGCAACGGGCAATATGTCTCTGTGTGGATTAAAAAAAGAGTGTCTGATAGCAGC"
    val dna = new DNA(s)
    val expected = Map('A' -> 20, 'T' -> 21, 'G' -> 17, 'C' -> 12)
    dna.nucleotideCounts should be (expected)
  }
}

Source

\$\endgroup\$
  • \$\begingroup\$ Please don't edit updates into your question after you've received answers. That invalidates the answers \$\endgroup\$ – Quill Jun 17 '16 at 1:51
  • \$\begingroup\$ @Quill but my intention was not to validate answers, if you look at the correct answer, it doesn't pass the tests which it should according to the code review policy hence I thought it would be good to do slight modification and write the passing code so that any newcomer like me isn't left in between. \$\endgroup\$ – CodeYogi Jun 17 '16 at 1:55
  • \$\begingroup\$ If you have an issue with the answers, please leave a comment on the answer. You shouldn't edit updated code into the question, you can, however, make a new question with the new code \$\endgroup\$ – Quill Jun 17 '16 at 1:57
2
\$\begingroup\$

You're right that this doesn't feel like .

To start, avoid mutable collections; mutability goes against functional programming principles.

This problem should be approached as some kind of fold operation: you start with some initial result (a map with zero counts), then transform it for each letter encountered in the string.

You shouldn't need to validate the string as a separate step; you can validate as you go.

A type annotation of the result would be a good idea.

If your only operation on a DNA sequence is to perform a nucleotide count, I don't think you need a class. A "static" function on a DNA object, called like val counts = DNA.nucleotideCounts("AACTG"), will do.

object DNA {
  def nucleotideCounts(dnaSequence: String): Map[Char, Int] = {
    val zero = Map('A' -> 0, 'T' -> 0, 'C' -> 0, 'G' -> 0)
    dnaSequence.foldLeft(zero)((counts, nucleotide) =>
      counts.updated(nucleotide, counts.get(nucleotide) match {
        case Some(n) => n + 1
        case None    => throw new RuntimeException()
      })
    )
  }
}

Or, you could implement the original interface using the fold technique:

class DNA(dnaSequence: String) {
  private val zero = Map('A' -> 0, 'T' -> 0, 'C' -> 0, 'G' -> 0)
  private val counts = dnaSequence.foldLeft(zero){ (counts, nucleotide) =>
    counts.updated(nucleotide, counts.get(nucleotide) match {
      case Some(n) => n + 1
      case None    => throw new RuntimeException()
    })
  }

  def nucleotideCounts(): Map[Char, Int] = counts
  def nucleotideCounts(nucleotide: Char): Int = counts.get(nucleotide).get
}
\$\endgroup\$
  • \$\begingroup\$ Wouldn't we doing same computation again and again? since we aren't storing counts here but computing them on fly? \$\endgroup\$ – CodeYogi Jun 17 '16 at 1:10
  • \$\begingroup\$ If you store the resulting Map[Char, Int], then you wouldn't be recomputing anything. \$\endgroup\$ – 200_success Jun 17 '16 at 1:11
  • \$\begingroup\$ Your second solution wouldn't handle the exception in case of invalid nucleotide. \$\endgroup\$ – CodeYogi Jun 17 '16 at 16:34
2
\$\begingroup\$

Encapsulation and information hiding

The parameterless nucleotideCounts method exposes the map of counts. If later you wanted to change the way you store the counts, for example you wanted to store in a database instead of a map in memory, you won't be able to do that, because you did not hide this information.

Exposing the map is especially suspicious since the map is mutable. Users of this class could circumvent the validation logic, for example delete nucleotides or add invalid nucleotides.

In any case, it would be better to declare the count map private.

Unnecessary getOrElse

Thanks to the validate method, and if we assume that users of the parameterless nucleotideCounts method will not abuse the class and delete nucleotides, it's guaranteed that count.get(c).get will always find a value, so you can replace all occurrences of count.get(c).getOrElse(0).

Mutable or not

It's not clear why you use a mutable map. Your current implementation and tests would work with an immutable map too.

If you really want to use a mutable map, then you can replace this:

count = count.updated(i, count.get(i).getOrElse(0) + 1)

with:

count.update(c, count.get(c).get + 1)

Naming

count is a not a great name for a collection type. For example the plural counts would be a more fitting name, which would also match the naming of your parameterless nucleotideCounts method.

You used multiple names for a character in a DNA string: i, chr, nucleotide. It would be easier to read the code if you used a consistent name. i is especially a bad name, as it's typically used in counting loops.

\$\endgroup\$
1
\$\begingroup\$

The following code passes your tests and uses immutable data structures.

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

class DNA(sequence: String) {
  if ((Set(sequence:_*) union DNA.nuclSet) != DNA.nuclSet) {
    throw new RuntimeException()
  }

  val nucleotideCounts = {
    val m0 = sequence.toSeq.groupBy(c => c).mapValues(_.size)
    DNA.nuclMap.map { case (k, v) => k -> m0.getOrElse(k, 0) }
  }
}

The DNA object is what is known as a companion object. In this case we might use it to store values that are the same across all instantiations of the DNA class .

Within the DNA class itself we start off by checking that the Char's in sequence belong to the set of allowable nucleotides.

Finally we calculate nucleotideCounts. The crux of the calculation rests in the use of groupBy followed with a mapValues. If you are unfamiliar with these methods I'd suggest playing around with them in the REPL as they come in handy.

\$\endgroup\$
  • \$\begingroup\$ There is no need to keep nuclSet in DNA, because nuclMap.keySet provides access to these values and avoids duplication. \$\endgroup\$ – Antot Jun 18 '16 at 12:47

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.