separate rules model from validation #179
Conversation
| s"invalid day of the week: $date is a $dateDayStr. Allowed days are $allowedDaysStr" | ||
| } | ||
| } | ||
| case class DaysOfWeekRule(allowedDays: List[DayOfWeek]) extends DateRule | ||
|
|
||
| case class WindowRule( | ||
| now: () => LocalDate, |
There was a problem hiding this comment.
now param could go away from here
| now: () => LocalDate, | ||
| maybeCutOffDay: Option[DayOfWeek], | ||
| maybeStartDelay: Option[Days], | ||
| maybeSize: Option[Days] |
There was a problem hiding this comment.
the last 3 params could be just the WindowRule object
Pull Request Test Coverage Report for Build 178
💛 - Coveralls |
| def contains(date: LocalDate) = { | ||
| val isOnOrAfterWindowStart = date.isAfter(start) || date.isEqual(start) | ||
| object DateValidator { | ||
| def validatorFor(now: () => LocalDate, dateRule: DateRule): LocalDate => ValidationResult[Unit] = dateRule match { |
There was a problem hiding this comment.
pattern match here makes more sense because DateRule is now a sealed trait - the compiler will warn us if we miss something.
just for background:
there are 3 main ways of doing polymorphism in scala -
- is inheritance (which creates a dep from the classes to the polymorphic code, but separates the implementation for each class from each other)
- is pattern matching (which reverses the dep - it's now from the blob of polymorphic code to the type, but the implementations are grouped together into one place)
- is type classes (which creates a dep from each classes implemention to both the class and the definition, so separates the implementation for each class)
3 produces the most independent code, but it uses implicits, with all their confusing aspects!
Overall I think the way you have proposed is the best solution for now, but we can always revisit.
|
|
||
| selectableWindow contains dateToValidate orFailWith errorMessage | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this way of splitting up is great, I feel like all the parts have a cohesive responsibliity now and should be easier to test.
I agree that moving stuff around will help a lot but I think in terms of dependencies between parts it's much better 🥇
There was a problem hiding this comment.
just as a side thought now the dependencies are reduced, in terms of organising into packages, we should think whether to group by function (window, days of week) or type (validator, rule, wire catalog) or a combination. I think it would depend on how the remaining connections go, and whether we are likely to make changes by function or by type. The compiler will check it all anyway, so it is just for our convenience how we organise it.
pvighi
changed the title
This is just @pvighi doing an experiment/prototype but I couldn't comment without making a PR. This would merge into the main branch not into Master.