feat: add module timing storage #53
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #53 +/- ##
============================================
- Coverage 65.67% 65.18% -0.50%
- Complexity 433 434 +1
============================================
Files 86 87 +1
Lines 1620 1660 +40
Branches 150 154 +4
============================================
+ Hits 1064 1082 +18
- Misses 521 540 +19
- Partials 35 38 +3 ☔ View full report in Codecov by Sentry. |
| */ | ||
| public Student(Name name, Phone phone, Email email, Address address, Set<Tag> tags, List<ModuleCode> modules) { | ||
| public Student(Name name, Phone phone, Email email, Address address, Set<Tag> tags, List<ModuleCode> modules, | ||
| List<ModuleTiming> moduleTimings) { | ||
| requireAllNonNull(name, phone, email, address, tags); |
There was a problem hiding this comment.
Would it be useful to instead have a Module that encapsulates ModuleCode and ModuleTiming. I think this approach might make more sense since the student object does not need to interact with ModuleCode and ModuleTiming directly, and can instead operate at a higher level of abstraction with just a list for Module.
We can consider doing this as a quality improvement in the next iteration!
There was a problem hiding this comment.
I was thinking about that too but I realised that we only instantiate each Module once and store them all inside the ModuleMap, which was why I didn't put ModuleTiming under Module.
blaukc
force-pushed
the
brandon/add-module-timing-storage
branch
from
d38e0a3 to
0e453ec
Compare
Adds the json adapted things for module storage. Mostly copypasting and refactoring after changing the Student constructor.
Will rebase and change target branch to master once the other PR is reviewed and merged
Closes: #49