| layout | title |
|---|---|
page |
Scala Style Guide |
On this page we publish feedback notes which are specific to individual asignments. For feedback which applies to coding style in general, visit the Scala Style Guide wiki page.
The following table indicates how often each of the issues occured during this assignment (the Scala Style Guide describes the first 12 issues).
#issue lukas
#1 2/23
#2 3/23
#3 12/23
#4 9/23
#5 7/23
#6 0/23
#7 0/23
#8 7/23
#9 1/23
#10 0/23
#11 2/23
#12 1/23
#4.1 11/23
#4.2 23/23
#4.3 10/23
#4.4 5/23
| #1 (casts) | x % |
| #2 (indent) | x % |
| #3 (line length) | x % |
| #4 (use locals) | x % |
| #5 (good names) | x % |
| #6 (common subexpr) | x % |
| #7 (copy-paste) | x % |
| #8 (semicolons) | x % |
| #9 (print stmts) | x % |
| #10 (return) | x % |
| #11 (vars) | x % |
| #12 (redundant if) | x % |
| #4.1 (weight / chars) | x % |
| #4.2 (::: vs ++) | x % |
| #4.3 (list matching) | x % |
| #4.4 (sort too often) | x % |
Every code tree (leaf or fork) contains the the full weight and list of characters. Therefore, implementing def weight and def chars does not require a recursive computation.
Lists can be concatenated with either :::, as most solutions do, or using the ++ operator. The latter has the advantage of being applicable to other sequence types (and also exists for other collection types), while ::: only exists for lists.
Some submissions make extensive use of isEmpty, head and tail instead of using pattern matches on lists. The corresponding code is longer and less elegant, for example
def count(ch: Char, counter: Int, list: List[Char]): Int = {
if (list.isEmpty || !list.contains(ch)) counter
else if (list.head.equals(ch)) count(ch, counter + 1, list.tail)
else count(ch, counter, list.tail)
}
is more easily written as follows:
def count(ch: Char, counter: Int, list: List[Char]): Int = list match {
case Nil => counter
case x :: xs =>
val newCounter = if (x == ch) counter + 1 else counter
count(ch, newCounter, xs)
}
Note that the call to list.contains(ch) was removed: it was counter-productive: the entire list is traversed at every iteration, therefore the first implementation has complexity O(n^2). The second implementation has complexity O(n).
Note that the counter parameter is not required, the method can be written as follows (although it won't be tail-recursive anymore):
def count(ch: Char, list: List[Char]): Int = list match {
case Nil => 0
case x :: xs =>
val increment = if (x == ch) 1 else 0
increment + count(ch, xs)
}
To sort the the list of frequencies, some solutions of makeOrderedLeafList call sort in every iteration - this is unnecessary, calling it one on the entire list would be enough. To avoid the problem, a helper method might be required. Example:
def makeOrderedLeafList(freqs: List[(Char, Int)]): List[Leaf] =
freqs.sortWith((a,b) => a._2 <= b._2) match {
[...] makeOrderedLeafList(someTail) [...]
}
The following table indicates how often each of the issues occured during this assignment (the Scala Style Guide describes the first 12 issues).
| #1 (casts) | 1 % |
| #2 (indent) | 12 % |
| #3 (line length) | 37 % |
| #4 (use locals) | 0 % |
| #5 (good names) | 13 % |
| #6 (common subexpr) | 59 % |
| #7 (copy-paste) | 54 % |
| #8 (semicolons) | 20 % |
| #9 (print stmts) | 1 % |
| #10 (return) | 0 % |
| #11 (vars) | 4 % |
| #12 (redundant if) | 0 % |
| #3.1 (union) | 52 % |
Instead of implementing union in the base class TweetSet and testing for isEmpty, a more elegant solution is to keep union abstract in the base class and provide an implementation in each subclass:
abstract class TweetSet {
def union(that: TweetSet): TweetSet
}
class Empty extends TweetSet {
def union(that: TweetSet): TweetSet = ???
}
class NonEmpty extends TweetSet {
def union(that: TweetSet): TweetSet = ???
}