Skip to content

Conversation

@joan-miralles
Copy link

@joan-miralles joan-miralles commented Dec 24, 2017

Copy link
Member

@JavierCane JavierCane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions added 🙂

@joan-miralles tell me if you want to apply these suggestions in this very same PR. If so, I would wait for them before merging it.


counter.countWords(nonEmptyText) shouldBe wordsNumber
}
"return the number of words given a non empty text with spaces" in {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good one!

Suggestion: Add other corner cases such as empty strings (0), punctuation, and so on 😬

def countWords(text: String): Int = {
0
if (text.isEmpty)
return 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return statement not needed. In fact, it's not Scala idiomatic to specify such clauses since we will always return the last statement in a code block. In order to remove it, we should add the else part of the if 🙂

def aggregateWords(text: String): Map[String, Int] = {
Map.empty
if (text.isEmpty)
return Map.empty
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as with the WordCounter case. You don't need to explicitly declare the return statements 🙂

Map.empty
if (text.isEmpty)
return Map.empty
var occurrences = Map[String, Int]()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to think about an alternative implementation without requiring mutability (var) 🙂

Hint: Instead of iterating over the words: Array[String] with a foreach, there're other methods which allow to accumulate a result between each array item iteration. Take a look at the scala.Array documentation 😬

@joan-miralles
Copy link
Author

Fixed! I've moved solution to exercise_solutions folder. (better if you want merge PR to public repository)

@JavierCane
Copy link
Member

Thanks for moving your solution proposal to its own package. It definitely help us out!

Merging!

@JavierCane JavierCane merged commit bbf2dbf into CodelyTV:master Jan 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants