Skip to content

review for skillbox#1

Open
MaxVodolazhsky wants to merge 1 commit intomainfrom
develop
Open

review for skillbox#1
MaxVodolazhsky wants to merge 1 commit intomainfrom
develop

Conversation

@MaxVodolazhsky
Copy link
Owner

No description provided.


public void moneyFromAccount(double amount) {
if (getBalance() >= amount) {
balance = getBalance() - amount;
Copy link
Owner Author

@MaxVodolazhsky MaxVodolazhsky Jan 20, 2022

Choose a reason for hiding this comment

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

лучше использовать переменную balance - напрямую.

this.balance = balance;
}

public void moneyToAccount(double amount) {
Copy link
Owner Author

Choose a reason for hiding this comment

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

В силу того, что в данном методе находимся внутри нашего класса - следовательно имеем доступ к переменной balance.
В данном случае лучше использовать переменную balance напрямую.
balance = balance + amount

}

//
// public void AccountsBalance(double balance)
Copy link
Owner Author

Choose a reason for hiding this comment

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

Данные закоменченные строки необходимо удалить. Не имейте такую привычку, так как при приеме на работу - на Вашем в любом случае будет какой либо анализатор кода. Из-за данной оплошности Вы скорее всего - не пройдете его проверку.
https://habr.com/ru/company/pvs-studio/blog/514124/
Если же в данном куске кода есть какой либо задел на новый функционал, то для данной задачи существует следующий формат комментариев:
// TODO нужно сделать что-то =)


public static void main(String[] args) {

Client Ip = new IpAccount(0);
Copy link
Owner Author

Choose a reason for hiding this comment

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

В ЯП Java есть строгие правила нейминга переменных, советую с ними ознакомиться:
https://khasang.io/courses/283142/lectures/4428018
(в данном случае переменная должна иметь название ip)

super(balance);
}

public void moneyToAccount(double amount) {
Copy link
Owner Author

Choose a reason for hiding this comment

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

В данном случае - нет смысла переопределять данный метод. Так как при использовании наследования реализация метода родителя внедряется в Ваш класс.
Советую немного почитать про один из 3 принципов ООП, а именно про наследование:
https://habr.com/ru/post/87205/

super.moneyToAccount(amount);
}

public void moneyFromAccount(double amount) {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Аналогичная ситуация.
см. предыдущий комментарий.


public class IpAccount extends Client {

private final double WITHDRAW_FEE = 0.01;
Copy link
Owner Author

Choose a reason for hiding this comment

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

Поля класса имеющие заданные значения - принято помечать ключевым словом static
В результате должно получится:
private static final double WITHDRAW_FEE = 0.01;
private static final double WITHDRAW_FEE2 = 0.005;

Так же, но это скорее по желанию. Значение процента комиссии 1% используется в двух классах это:
IpAccount и UrAccount. Следовательно присутствует дублирование объявление переменной WITHDRAW_FEE. Предлагаю данную переменную вынести в абстрактный класс Client c модификатором доступа protected.

Должно получиться нечто подобное:

public abstract class Client {
     protected static final double WITHDRAW_FEE = 0.01;
...
}

}
}

public void moneyFromAccount(double amount) {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Данное переопределение метода - излишне.

return balance;
}

public abstract double getFizAccount();
Copy link
Owner Author

Choose a reason for hiding this comment

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

Советую почитать более подробно про абстрактные методы:
http://developer.alexanderklimov.ru/android/java/abstract.php

Просмотрев классы наследники - убедился в том, что пометка abstract этих 3 методов теряет смысл, так как реализация у всех классов этих методов единственная (return 0;)
Предлагаю данную реализацию внедрить в класс родитель (Client), а в классах наследниках, данные реализации - удалить. Таким образом избавимся от дублирование кода, но не потеряем в функционале.

Так же вопрос, для чего Вам необходимы данные методы, не могли бы пояснить?

super(balance);
}

public void moneyToAccount(double amount) {
Copy link
Owner Author

Choose a reason for hiding this comment

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

Данное переопределение не имеет смысла если используете реализацию класса родителя. Ее можно удалить.

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.

1 participant