Skip to main content
Code Review

Return to Answer

edited body (comment edited Sep 17, 2016 at 10:00)
Source Link
200_success
  • 145.5k
  • 22
  • 190
  • 478

The heart of your program is the for loop. It's not very good functional programming, though. Ideally, the entire exercise should revolve around one line of code that looks like this:

The heart of your program is the for loop. It's not very good functional programming, though. Ideally, the entire exercise should revolve one line of code that looks like this:

The heart of your program is the for loop. It's not very good functional programming, though. Ideally, the entire exercise should revolve around one line of code that looks like this:

hide helpers
Source Link
200_success
  • 145.5k
  • 22
  • 190
  • 478
val wordValue = {
 val ordinals = ('A' to 'Z').zipWithIndex.toMap
def wordValue(word:String): Int = {
 => word.length + word.map(ordinals).sum
}
val isTriangularNumber = {
 val triangularNumbers = Stream.from(1).scan(0)(_ + _)
def isTriangularNumber(n:Int): Boolean = {
 => triangularNumbers.find(_ >= n).get == n
}
val inputFile = Source fromFile Paths.get(args(0)).toUri
val words = "\"([^\"]*)\"".r findAllMatchIn(inputFile.mkString) map(_.group(1))
 
val count = words.count(word => isTriangularNumber(wordValue(word)))
println("The answer is: " + count)
val ordinals = ('A' to 'Z').zipWithIndex.toMap
def wordValue(word:String): Int = {
  word.length + word.map(ordinals).sum
}
val triangularNumbers = Stream.from(1).scan(0)(_ + _)
def isTriangularNumber(n:Int): Boolean = {
  triangularNumbers.find(_ >= n).get == n
}
val inputFile = Source fromFile Paths.get(args(0)).toUri
val words = "\"([^\"]*)\"".r findAllMatchIn(inputFile.mkString) map(_.group(1))
 
val count = words.count(word => isTriangularNumber(wordValue(word)))
println("The answer is: " + count)
val wordValue = {
 val ordinals = ('A' to 'Z').zipWithIndex.toMap
 (word:String) => word.length + word.map(ordinals).sum
}
val isTriangularNumber = {
 val triangularNumbers = Stream.from(1).scan(0)(_ + _)
 (n:Int) => triangularNumbers.find(_ >= n).get == n
}
val inputFile = Source fromFile Paths.get(args(0)).toUri
val words = "\"([^\"]*)\"".r findAllMatchIn(inputFile.mkString) map(_.group(1))
 
val count = words.count(word => isTriangularNumber(wordValue(word)))
println("The answer is: " + count)
Source Link
200_success
  • 145.5k
  • 22
  • 190
  • 478

The organization of the program is rather haphazard:

  1. It starts with a word value calculation function
  2. Then it checks for the existence of the input file
  3. Then defines the ordinals map (which the first function depends on!)
  4. Then it creates a triangleNumbers map
  5. Then it reads the words from the input file
  6. Then it counts the words that pass the triangularity test

Interspersed among that are three unit tests.

Surely these definitions could be grouped together more logically.


makeTriangleNumber is altogether wrongly named. It actually has nothing to do wth triangular numbers. Rather, it sums the values of the letters of a word. I would call it wordValue instead.

Within the makeTriangleNumber function, .reduceLeft(_ + _) could just be written as .sum.

Personally, I would simplify the definition of ordinals by taking advantage of Seq.zipWithIndex, then compensate for the off-by-one difference by adding the length of the word. (That may be a controversial move, since ordinals would no longer match the problem description.)


For extracting the words from the file, I suggest using Regex.findAllMatchIn(source:CharSequence), which gives you an Iterator of regular expression matches.


The heart of your program is the for loop. It's not very good functional programming, though. Ideally, the entire exercise should revolve one line of code that looks like this:

words.count(word => isTriangularNumber(wordValue(word)))

How should isTriangularNumber be defined? In your loop, you're discovering triangular numbers like this:

var tmp = Stream.from(triangleNumbers.max._2 + 1)
 .map((x) => (x * (x + 1) / 2, x))
 .takeWhile(_._1 <= candidate)

... using the triangleNumbers map for memoization. I propose a more elegant definition — a lazy stream:

val triangularNumbers = Stream.from(1).scan(0)(_ + _)

Rather than the given formula, this definition relies on the recurrence principle that makes the numbers "triangular" in the first place.

Suggested solution

val ordinals = ('A' to 'Z').zipWithIndex.toMap
def wordValue(word:String): Int = {
 word.length + word.map(ordinals).sum
}
val triangularNumbers = Stream.from(1).scan(0)(_ + _)
def isTriangularNumber(n:Int): Boolean = {
 triangularNumbers.find(_ >= n).get == n
}
val inputFile = Source fromFile Paths.get(args(0)).toUri
val words = "\"([^\"]*)\"".r findAllMatchIn(inputFile.mkString) map(_.group(1))
 
val count = words.count(word => isTriangularNumber(wordValue(word)))
println("The answer is: " + count)
lang-scala

AltStyle によって変換されたページ (->オリジナル) /