Refactor FindingsReport + FileBasedFindingsReport #2454
Conversation
| } | ||
|
|
||
| /** Formats the debt time to a human readable format. */ | ||
| fun formatDebtTime(): Debt { |
There was a problem hiding this comment.
Shouldn't the overflow validation be moved to the toString() method or the function should be renamed to format and return just a String and toString just calls it?
Or should the overflow calculation be moved to the constructor? This introduces a new public api function.
There was a problem hiding this comment.
I wouldn't want to add the validation to the constructor.
The values would have to be mutable, which I wanted to avoid.
Maybe we can come up with a better name than formatDebtTime for this function.
There was a problem hiding this comment.
Maybe we move this function to the companion object and call it Debt.recalculateOverflow(instance)?
There was a problem hiding this comment.
I think that the plus operator should handle this problem. It's his responsability to create valid Debt instances.
If we want to do something in the constructor is to check that Debt is valid. And if it's not, throw an exception. But I think that it's not worth it.
| append(issuesString.yellow()) | ||
| } | ||
| val overallDebt = debtList.reduce { acc, d -> acc + d } | ||
| append("Overall debt: $overallDebt".format("\n")) |
There was a problem hiding this comment.
I think the overallDebt.formatDebtTime (Debt.recalculateOveflow(overallDebt)) call is missing here?
There was a problem hiding this comment.
This is a bug. Thanks! I shall add a test for it.
The toString() method should call the formatDebtTime() function here.
| } | ||
|
|
||
| /** Formats the debt time to a human readable format. */ | ||
| fun formatDebtTime(): Debt { |
There was a problem hiding this comment.
I think that the plus operator should handle this problem. It's his responsability to create valid Debt instances.
If we want to do something in the constructor is to check that Debt is valid. And if it's not, throw an exception. But I think that it's not worth it.
Agreed. |
It's done by overriding the plus operator function in the Debt class.
This ensures that the debt is always inside the time boundaries.
|
I rebased the branch and incorporated your feedback in the last commit. This PR is now ready to merge. |
No description provided.