New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests refactorings #1

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@etorreborre

etorreborre commented Feb 8, 2014

I read your blog post and I propose a few changes to your specification essentially to avoid getting errors instead of failures:

  • added several tests under the same should section
  • used the latest specs2 with the possibility to pass functions
    to beSome
  • created custom matchers to deal with the comparison of Options
  • note: find should return an Option
tests refactorings
- added several tests under the same should section
- used the latest specs2 with the possibility to pass functions
  to beSome
- created custom matchers to deal with the comparison of Options
- note: find should return an Option
@jorilytter

This comment has been minimized.

Show comment
Hide comment
@jorilytter

jorilytter Feb 10, 2014

Owner

Thanks for the good suggestions, I won't be merging this as is but will pick up on your suggestions

  • tests under the same should section: This was on my refactoring list and will be doing this
  • latest specs2 & custom matchers: Won't be doing this, yet, probably later
  • find should return an Option: Implementation was like this before, commit 97586f7, but I changed it to it's current implementation. I'd love to hear arguments why it should return Option and if find returns Option then the other functions that return a Task should probably too return Option to keep the code consistent.
Owner

jorilytter commented Feb 10, 2014

Thanks for the good suggestions, I won't be merging this as is but will pick up on your suggestions

  • tests under the same should section: This was on my refactoring list and will be doing this
  • latest specs2 & custom matchers: Won't be doing this, yet, probably later
  • find should return an Option: Implementation was like this before, commit 97586f7, but I changed it to it's current implementation. I'd love to hear arguments why it should return Option and if find returns Option then the other functions that return a Task should probably too return Option to keep the code consistent.
@etorreborre

This comment has been minimized.

Show comment
Hide comment
@etorreborre

etorreborre Feb 10, 2014

If you return Option then you won't be exposed to runtime exceptions (BTW tasks.get(id).get is equivalent to tasks(id)). The type system forces you to acknowledge that there might not be a task for a given id and you have to declare what to do then (return an error message for example).

This might seems a bit clumsy at first because you have this Option[Task] to manipulate all the time. But this is where combinators and for come to the rescue. You can:

  • map. e.g.: find(id).map(t => getCreationTime(t))
  • flatMap. e.g.: find(id).flatMap(t => getNext(t.id))
  • fold. e.g.: find(id).fold(found => "found it", "couldn't find "+id)
  • for. e.g: for { t1 <- find(id1); t2 <- find(id2) } yield addDependency(t1, t2)

etorreborre commented Feb 10, 2014

If you return Option then you won't be exposed to runtime exceptions (BTW tasks.get(id).get is equivalent to tasks(id)). The type system forces you to acknowledge that there might not be a task for a given id and you have to declare what to do then (return an error message for example).

This might seems a bit clumsy at first because you have this Option[Task] to manipulate all the time. But this is where combinators and for come to the rescue. You can:

  • map. e.g.: find(id).map(t => getCreationTime(t))
  • flatMap. e.g.: find(id).flatMap(t => getNext(t.id))
  • fold. e.g.: find(id).fold(found => "found it", "couldn't find "+id)
  • for. e.g: for { t1 <- find(id1); t2 <- find(id2) } yield addDependency(t1, t2)
@jorilytter

This comment has been minimized.

Show comment
Hide comment
@jorilytter

jorilytter Feb 12, 2014

Owner

I can see your point regarding runtime exceptions and if this was something else than a simple experiment application with Scala or if the application needed to handle more than one task at a time I'd use Option[Task] but for now I'll settle with the current implementation. The Task vs. exception is currently dealt with function taskResponse in Application object.

Thanks for the note tasks.get(id).get == tasks(id), I'll definetly have to those replaced.

Owner

jorilytter commented Feb 12, 2014

I can see your point regarding runtime exceptions and if this was something else than a simple experiment application with Scala or if the application needed to handle more than one task at a time I'd use Option[Task] but for now I'll settle with the current implementation. The Task vs. exception is currently dealt with function taskResponse in Application object.

Thanks for the note tasks.get(id).get == tasks(id), I'll definetly have to those replaced.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment