Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Experience" trophy score threshold adjustments #260

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TurtleCode84
Copy link

@TurtleCode84 TurtleCode84 commented Feb 12, 2024

Adjusted two of the score thresholds for the AccountDurationTrophy to be more accurate estimates of the durations in the code comments.

"Seasoned Veteran": 70 changed to 73 (20 years)
"Master Dev": 40 changed to 37 (10 years)

See #258 for additional discussion

Adjusted a few of the score thresholds for the AccountDurationTrophy to be more accurate estimates of the durations in the code comments.
Copy link

vercel bot commented Feb 12, 2024

@TurtleCode84 is attempting to deploy a commit to the ryo-ma's Team Team on Vercel.

A member of the Team first needs to authorize it.

@lulunac27a
Copy link

10 years is approximately 3652 to 3653 days and 20 years is approximately 7304 to 7305 days.
Divide this by 100 gives approximately 37 for 10 years and 73 for 20 years.

@TurtleCode84
Copy link
Author

10 years is approximately 3652 to 3653 days and 20 years is approximately 7304 to 7305 days.

Right; in terms of math, I think I did something like 37 * 100 / 365.25 = 10.1300479124, which is closer to 10 years than 36 * 100 / 365.25 = 9.85626283368 (also assuming we're only using whole numbers for the thresholds).

Same concept for the 20 years.

@TurtleCode84
Copy link
Author

@alexcastrodev Requesting review and any suggestions for improvement.

@alexcastrodev
Copy link
Collaborator

@alexcastrodev Requesting review and any suggestions for improvement.

Hi, Thanks for pinging me and taking time to improve the project :)
For the calculation, @ryo-ma knows behaviour of calculation better than me.

I have two points in this kind of solution:

  1. It must be a Date / Intl calculation. I dont think we need every year review a PR to change a fixed variable.
  2. After fix this calculation using native Date API, this must have testing for ensure we will not back again to this problem.

If you want to do it, go ahead.

@ryo-ma
Copy link
Owner

ryo-ma commented Mar 18, 2024

@TurtleCode84 I understand the argument.
However, I felt it would be better to modify the logic that calculates Duration Days.

const durationDays = Math.floor(
durationTime / (1000 * 60 * 60 * 24) / 100,
);

It's my fault for merging the above code.

@ryo-ma
Copy link
Owner

ryo-ma commented Mar 18, 2024

@bhavberi I feel that there is no need to calculate DurationDays by shortening it to 100 days.

Copy link

@Imran-imtiaz48 Imran-imtiaz48 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review and Improvement Suggestions:
Consistency: Ensure consistency in naming conventions and formatting throughout your codebase. It helps maintain readability and reduces cognitive load when reviewing or modifying code.

Commenting: Comments like // 20 years and // 10 years are helpful for understanding the intent behind numeric values. Ensure these comments accurately reflect the logic or condition being set.

Testing: After making these changes, it's important to thoroughly test your code to ensure that the logic behaves as expected under various scenarios.

Version Control: Use version control effectively to track changes like these, making it easier to revert if necessary and to collaborate with others.

These adjustments should help in maintaining and improving the quality of your codebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants