Skip to content

Conversation

lbrdev
Copy link

@lbrdev lbrdev commented Jan 10, 2017

to check 1, 2

@didva didva self-assigned this Jan 10, 2017
* Project: oop.pr1
*/
public class Accountant extends Employee {
private static int countAccountant;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need this field?

return getRatioOfHours() * getSalary();
}

public int overallSalary(Employee[] employees) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to store array/list of employees in the field and do not pass them through argument.

return getRatioOfHours() * getSalary();
}

public int overallSalary(Employee[] employees) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this method returns integer?

}

public int overallSalary(Employee[] employees) {
double i = 0.;
Copy link
Contributor

Choose a reason for hiding this comment

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

i is not the best name for this purpose.

}

@Override
public double calculateSalary() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you override method with exactly the same implementation? I'd prefer to make base method abstract.

* Created by lbrdev on 04.01.2017.
* Project: oop.pr1
*/
public class FractionNumberOperations {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move all these operations inside the FractionNumber class.

}

public double decimalValue() {
return ((double) dividend) / ((double) divisor);
Copy link
Contributor

Choose a reason for hiding this comment

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

The only one cast will be enough.

this.dividend = dividend;
this.divisor = DEFAULT_DIVISOR;
} else {
this.dividend = dividend;
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need to assign these fields. They will be overridden on the next few lines.

}

private int gcd(int dividend, int divisor) {
int maxValue;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to assign values like:

 int maxValue = dividend;
 int minValue = divisor;

And remove one condition.


public FractionNumber(int dividend) {
this.dividend = dividend;
this.divisor = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use default value instead of 1

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.

2 participants