-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
added salary calculation logic #1339
base: master
Are you sure you want to change the base?
added salary calculation logic #1339
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on the Pull Request! Just a few minor suggestions for improvement. Remember to use your variables consistently to keep the code clean, and consider introducing constants for any magic numbers to make your code more readable and maintainable. Also, think about giving your methods descriptive names that clearly indicate their purpose. Lastly, for any constants in your code, stick to the naming conventions to enhance clarity. Keep up the good work! 👍💻
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
private static final int PAY_PER_HOUR_INDEX = 3; | ||
private static final String DATE_FORMAT = "dd.MM.yyyy"; | ||
|
||
public static final DateTimeFormatter formatter = DateTimeFormatter.ofPattern(DATE_FORMAT); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider making the formatter a constant field. The naming convention for constants in Java is to use all uppercase with underscores to separate words. So the formatter should be named like FORMATTER.
.append(dateTo) | ||
.append(System.lineSeparator()); | ||
|
||
salaryInfo(names, data, formatDateFrom, formatDateTo, stringBuilder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method name 'salaryInfo' is not very informative. Consider renaming it to something more descriptive of its function, such as 'calculateAndAppendSalaries'.
&& (date.isBefore(formatDateTo) || date.isEqual(formatDateTo))) { | ||
Integer workHours = Integer.parseInt(employeeData[WORK_HOURS_INDEX]); | ||
Integer payPerHour = Integer.parseInt(employeeData[PAY_PER_HOUR_INDEX]); | ||
int salaryPerDay = workHours * payPerHour; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any magic numbers should be constants. The indexes you're using (0 for DATE_INDEX, 1 for NAME_INDEX, etc.) are good examples of converting magic numbers to constants. However, you should also consider making 'salaryPerDay' calculation more clear by introducing a constant if there's a fixed multiplier or any other number used in the calculation that isn't already a constant.
} | ||
} | ||
stringBuilder | ||
.append(names[i]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're using 'names[i]' here, but you already have the same value stored in 'employeeName'. It would be more consistent to use 'employeeName' here as well.
No description provided.