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

Solution #1308

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

Conversation

misha-doroshenko
Copy link

No description provided.

Copy link

@liliia-ponomarenko liliia-ponomarenko left a comment

Choose a reason for hiding this comment

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

Great job! Let's improve your solution ;)


public Employee(String name) {
this.name = name;
this.balance = 0;

Choose a reason for hiding this comment

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

0 - int is initialized to 0 by default


public class DateService {
public static boolean dateInRange(String date, String fromDate, String toDate) {
DateTimeFormatter formatter = DateTimeFormatter.ofPattern("dd.MM.yyyy");

Choose a reason for hiding this comment

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

Let's make this variable a constant

import java.time.LocalDate;
import java.time.format.DateTimeFormatter;

public class DateService {

Choose a reason for hiding this comment

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

DateTimeUtil

Comment on lines +14 to +15
return !dateFormatted.isBefore(fromDateFormatted)
&& !dateFormatted.isAfter(toDateFormatted);

Choose a reason for hiding this comment

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

logical NOT operator is difficult to read, so avoid using them if possible

public int getBalance() {
return balance;
}

Choose a reason for hiding this comment

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

Suggested change

package core.basesyntax;

public class EmployeeService {
public static Employee getEmployeeByName(Employee[] employees, String name) {

Choose a reason for hiding this comment

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

Don't use static methods in your solution
Static methods are in general a bad practice. Let's better create an instance of a class which method you want to call.

Comment on lines +7 to +9
for (int i = 0; i < names.length; i++) {
employees[i] = new Employee(names[i]);
}

Choose a reason for hiding this comment

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

Don’t create redundant loops. Two is enough.
The more loops you have, the slower your code may get, so try to solve task with minimum amount.
Now I see 4 loops in the solution, think about how to refactor it, and improve the performance of the solution.

}

for (String dataRow: data) {
String[] dataRowArray = dataRow.split(" ");

Choose a reason for hiding this comment

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

Any magic numbers and symbols should be constants. Check all your solution.

@@ -2,6 +2,33 @@

public class SalaryInfo {
public String getSalaryInfo(String[] names, String[] data, String dateFrom, String dateTo) {
return null;
Employee[] employees = new Employee[names.length];

Choose a reason for hiding this comment

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

This method is too big, can you encapsulate the logic in private methods?

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.

3 participants