#2363 Calculate player's recovery#2450
Conversation
# Conflicts: # src/main/java/core/gui/comp/table/HOTableModel.kt
# Conflicts: # src/main/resources/release_notes.md
| var ret = new ArrayList<HODateTime>(); | ||
| for (HODateTime dailyUpdate : getDailyUpdates()) { | ||
| while (dailyUpdate.isAfter(from)) { | ||
| dailyUpdate = dailyUpdate.minus(7, ChronoUnit.DAYS); |
There was a problem hiding this comment.
Please add a constant for the 7 instead of using a "magic number".
And and use it in every place where this is meant.
There was a problem hiding this comment.
Question: what is magic in writing Minus(7 days) ?
There was a problem hiding this comment.
I commented on the literal value 7 because it is a so-called “magic number”.
A magic number is a numeric value that appears directly in the source code without explaining its meaning. Even if the value itself is obvious mathematically, the intention behind it may not be immediately clear from the surrounding code.
For example, 7 could mean “days per week”, but it could also represent some business-specific limit, an offset, a retry count, or something else. By extracting it into a named constant, the code becomes more self-explanatory:
private static final int DAYS_PER_WEEK = 7;Using a constant improves readability, makes the intention explicit, and avoids repeating the same unexplained value in multiple places. It also makes future changes easier, because the value only has to be changed in one place if the business rule ever changes.
So the point is not that the number 7 is hard to understand, but that the code should communicate why this value is used in this specific context.
There was a problem hiding this comment.
For reasons i don't understand the ChronoUnit.Weeks is not supported. Would you program .minus(ONE weeks) and define
private static final int ONE = 1;
I don't think so....
There was a problem hiding this comment.
I think the reason is, that Instant is a point in time as defined seconds after or before 1970-01-01T00:00:00Z.
ChronoUnit means the calendar system and that system has summer and winter time depending on the timezone you are living in.
It is not clear by definition when you say "7 days after an Instant" if you meant 604.800 seconds after the Instant.
Or, if the Instant is for the 2026-03-29T00:00:00 (Germany) then you have the UTC+1 (winter time) as the start point.
604.800 after that Instant is very clear,
but 7 days are that Instant could mean 2026-04-05T00:00:00 (summer time UTC+2) or maybe 2026-04-05T01:00:00 (summer time UTC+2).
Anyway, I found it also very interesting that the API of Java here is very "restricted". :-)
I think for your calculations calendar days are meant, because Hattrick is calendar based for all match days and so on.
HODateTime uses internal an Instant and this might have been a ZonedDateTime with a specific ZoneId.
But I think that is a major refactoring...
There was a problem hiding this comment.
Would you program .minus(ONE weeks) and define
private static final int ONE = 1;
I don't think so....
For a 1 I would write 1 for sure.
But sometimes it may be important to read it explicitly and then I would use:
NumberUtils.INTEGER_ONE or NumberUtils.LONG_ONE from package org.apache.commons.lang3.math.
BigDecimal.ONE also exists.
We have that constants available in our project.
| /** | ||
| * Health calculation based on Schum formula (<a href="https://www82.hattrick.org/Forum/Read.aspx?t=17404127&n=6&v=0&mr=0">...</a>) | ||
| */ | ||
| public class Injury { |
There was a problem hiding this comment.
What about providing a test for that class?
There was a problem hiding this comment.
Good idea - will be done soon...
sgcr
left a comment
sgcr
left a comment
There was a problem hiding this comment.
Please have a look at my comments.
|
Thanks - Almost all comments have been incorporated |
| return whenSlightlyInjured; | ||
| } | ||
|
|
||
| public boolean getIsInvalid() { |
| * Date is null if player is healthy | ||
| */ | ||
| private HODateTime whenHealthy; | ||
| /** |
| return getInjury().getTypeOfEstimate(); | ||
| } | ||
|
|
||
| public Boolean isInvalid() { |
| private static double calcMedicianFactor(int doctorLevel) { | ||
| double x1Factor = 0.2124; | ||
| double x0Factor = 1; | ||
| return (x1Factor * (double) doctorLevel + x0Factor) / (x0Factor + 5 * x1Factor); |
| private static double calcMedicianFactor(int doctorLevel) { | ||
| double x1Factor = 0.2124; | ||
| double x0Factor = 1; | ||
| return (x1Factor * (double) doctorLevel + x0Factor) / (x0Factor + 5 * x1Factor); |
There was a problem hiding this comment.
5 should be a constant instead of magic number.
| /** | ||
| * Health calculation based on Schum formula (<a href="https://www82.hattrick.org/Forum/Read.aspx?t=17404127&n=6&v=0&mr=0">...</a>) | ||
| */ | ||
| public class Injury { |
There was a problem hiding this comment.
Class should be package private if the sole constructor is also package private.
| */ | ||
| private boolean isInvalid = false; | ||
|
|
||
| public enum TypeOfEstimate { |
There was a problem hiding this comment.
When Injury is only the calculation and all values are delegated to Getters in Player then this enum should be a general public enum outside that implementation class. Maybe InjuryEstimationType
| public enum TypeOfEstimate { | ||
| REALISTIC_ESTIMATE, | ||
| OPTIMISTIC_ESTIMATE, | ||
| PESSIMISTIC_ESTIMATE, |
| PESSIMISTIC_ESTIMATE, | ||
| } | ||
|
|
||
| private TypeOfEstimate typeOfEstimate = TypeOfEstimate.REALISTIC_ESTIMATE; |
| return regainerFactor * calcAgeFactor(player.getAgeAtDate(dateTime).toDouble()) * calcMedicianFactor(doctorLevel); | ||
| } | ||
|
|
||
| private void calculateRecovery(Player player) { |
There was a problem hiding this comment.
Method is very long: Could it be separated in parts of different sub functions?
| /** | ||
| * True if the player is an invalid (no recovery possible) | ||
| */ | ||
| private boolean isInvalid = false; |
There was a problem hiding this comment.
Suggestion: chronicInvalid as a name because invalid is very general and could be a state of the class instance itself.
2 participants
changes proposed in this pull request:
src/main/resources/release_notes.md...