start exam implementation: with the notate method, driven by test#5
start exam implementation: with the notate method, driven by test#5bkuchcik wants to merge 11 commits into
Conversation
…ation, not the exam. Start the refactoring
…project is not officilay supported in intellij today, job must be done manually)
…is. Some issues remains with junit 5, spring integration and kotlin
| import java.math.BigDecimal | ||
|
|
||
| data class Exam(val id: Long) { | ||
| fun validateNotation(notation: BigDecimal): Either<InvalidNotationForThisExamException, Unit> = |
There was a problem hiding this comment.
In English, this would be a grade :)
| fun validateNotation(notation: BigDecimal): Either<InvalidNotationForThisExamException, Unit> = | ||
| when (notation) { | ||
| in (0.toBigDecimal()..20.toBigDecimal()) -> Either.right(Unit) | ||
| else -> Either.left(InvalidNotationForThisExamException("This notation, $notation, is not in bound")) |
There was a problem hiding this comment.
I'd probably call it a GradeOutOfBoundsException
| } | ||
| } | ||
|
|
||
| class InvalidNotationForThisExamException(msg: String?) : RuntimeException(msg) |
There was a problem hiding this comment.
Kotlin exceptions are unchecked, so simply Exception is sufficient unless cross-compatibility with Java is required.
| } | ||
| } | ||
|
|
||
| class NotEvaluatedException(msg: String? = null, throwable: Throwable? = null) : RuntimeException(msg, throwable) No newline at end of file |
There was a problem hiding this comment.
A default message is probably preferable to null, yes?
| else -> Either.left(NotEvaluatedException()) | ||
| } | ||
|
|
||
| fun notate(exam: Exam, note: BigDecimal): Either<InvalidNotationForThisExamException, Student> = |
| studentsRepository.findStudentById(studentId = 1).flatMap { student -> | ||
| examsRepository.findExamById(examId = notateExam.examId).flatMap { exam -> | ||
| student.notate(exam, notateExam.note).flatMap { updatedStudent -> | ||
| studentsRepository.save(updatedStudent) |
There was a problem hiding this comment.
There are too many levels of abstraction here and it is not clear what this method is doing. Remember the SRP :)
There was a problem hiding this comment.
What would you remove there is only two call and entity method.
Maybe you do not like flatMap with either, but I don't see how to do it shorter than that
There was a problem hiding this comment.
Maybe by giving up either, but I want to use it to handle errors
There was a problem hiding this comment.
Mostly that we jump from a student into an exam into a grade for that exam and student. all the while nesting. Exam doesn't depend on student, so the nesting is odd.
There was a problem hiding this comment.
Nesting helps to get the values directly. The other solution is to do if(isRight()=> toOption().get()) etc..etc but I really don't like that solution (I tested it)
There was a problem hiding this comment.
I searched a lot, and normally the following code is better and compile (I need to check at home)
notateExam.also {
ForEither<RepositoryException>() extensions {
tupled(
studentsRepository.findStudentById(studentId),
examsRepository.findExamById(examId)
)
}.flatMap { (student, exam) ->
student.notate(exam, note).flatMap { studentsRepository.save(it) }
}.mapLeft { NotateExamException(throwable = it) }
}There was a problem hiding this comment.
It does not work, please close it, it is useless
| class StudentsInteractor(private val studentsRepository: StudentsRepository, private val examsRepository: ExamsRepository) : Students { | ||
|
|
||
| override fun notate(notateExam: NotateExam): Either<NotateExamException, Unit> = | ||
| studentsRepository.findStudentById(studentId = 1).flatMap { student -> |
There was a problem hiding this comment.
your studentId is defined in the notateExam class, we should probably use that otherwise your student 1 will get all the grades
There was a problem hiding this comment.
I already noticed it in my comment, but thx
| val studentsInteractor = StudentsInteractor(studentRepository, examsRepository) | ||
| studentsInteractor.notate20().apply { | ||
| it("should notate an existing exam with a note of 20") { | ||
| isRight() `should be` true |
There was a problem hiding this comment.
The actual grade isn't checked here. How do we know it's 20?
There was a problem hiding this comment.
Yes this test should be deleted because check is done in the last test
| typealias Grades = Map<Exam, Grade> | ||
|
|
||
| data class Student(val id: Long, val grades: Grades = emptyMap()) { | ||
| fun getNotation(exam: Exam): Either<NotEvaluatedException, BigDecimal> = |
| else -> Either.left(NotEvaluatedException()) | ||
| } | ||
|
|
||
| fun notate(exam: Exam, note: BigDecimal): Either<InvalidNotationForThisExamException, Student> = |
There was a problem hiding this comment.
Oops should be grade not notate
There was a problem hiding this comment.
And a Grade not a BigDecimal
| it("should retrieve exam by id") { | ||
| verify { examsRepository.findExamById(5) } | ||
| } | ||
| it("should save student to the repository with the expected notation of 20 for this exam") { |
2 participants
No description provided.