-
Notifications
You must be signed in to change notification settings - Fork 145
Review #8
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
base: master
Are you sure you want to change the base?
Review #8
Conversation
velllum
left a comment
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.
Тестовое задание на позицию код-ревьюера Яндекс.Практикум
| @@ -1,84 +1,97 @@ | |||
| import datetime as dt | |||
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.
Беглый анализ кода.
-
Классы желательно делить на несколько файлов (но не обязательно), у каждого должен быть свой сценарий работы, дальше с ними можно взаимодействовать при помощи импортов
import. -
Весь код не комментирован, а если да то, не правильно почитать тут, как привольно надо https://www.python.org/dev/peps/pep-0257/
-
Отсутствие
typingтайпитингов (указателей типов), для лучшего понимания что возвращается или храниться. -
Нет реализации клиентской части для проверки работоспособности написанного кода
-
Отсутствие тестов (пишутся в отдельной папке tests, с обязательным первым словом в имени файла
test). "Должен быть по условию технического задания. "
| @@ -1,84 +1,97 @@ | |||
| import datetime as dt | |||
| from datetime import datetime as dt | |||
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.
import datetime as dt для короткого обращения к свойствам и методам библиотеки datetime изменить импорт на более уточняющий from datetime import datetime as dt
| dt.datetime.now().date() if | ||
| not | ||
| date else dt.datetime.strptime(date, '%d.%m.%Y').date()) | ||
| self.date = dt.now().date() if not date else dt.strptime(date, '%d.%m.%Y').date() |
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.
- Укороченный вариант передачи данных свойству, уточнением импорта импорта.
- Удаления не нужных в данном случаи скобок.
- За счет этих изменений, можно вывести, тернарное условие в одну строку, не превышая лимит символов.
| def get_today_stats(self): | ||
| """- Комментарий""" | ||
| today_stats = 0 | ||
| for Record in self.records: |
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.
- В цикле for итератор не должен определяться с заглавной буквы.
- Не должен быть назван именем класса.
- Это имя уже зарезервировано, самим классом Record.
|
|
||
| for record in self.records: | ||
| if record.date == dt.now().date(): | ||
| today_stats += record.amount |
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.
Добавление в счетчик значения можно указывать не явным оператором увеличения (сложения) +=
|
|
||
| elif currency_type == 'eur': | ||
| cash_remained /= EURO_RATE | ||
| cash_remained /= euro_rate |
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.
Имена параметров не должны совпадать с именами аргументов класса.
| elif currency_type == 'rub': | ||
| cash_remained == 1.00 | ||
| currency_type = 'руб' | ||
| cash_remained = 1.00 |
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.
После проверки должно происходить не сравнение, а присвоение через указатель одного равно =
| return f''' | ||
| На сегодня осталось {result} | ||
| {currency_type}''' | ||
|
|
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.
- Строку можно прописывать в тройные кавычки.
- Таким механизмом можно занять несколько строк, без указателей переноса на другую строку через слэш.
- По правилу в f-строку нельзя вносить логические операции.
| ' твой долг - {0:.2f} {1}'.format(-cash_remained, currency_type) | ||
| return f''' | ||
| Денег нет, но вы держитесь ;) : | ||
| твой долг - {-cash_remained:.2f} {currency_type}''' |
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.
Строку можно прописывать в тройные кавычки.
Таким механизмом можно занять несколько строк, без указателей переноса на другую строку через слэш.
В данной задаче мы не используем устаревший вариант подстановки переменных через функцию str.format(), а применяем подстановку через f-строку
| week_stats = 0 | ||
| today = dt.datetime.now().date() | ||
| today = dt.now().date() | ||
|
|
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.
После изменения импорта, к классу .datetime можно теперь обращать более в укороченном варианте.
velllum
left a comment
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.
review добавление
| def get_today_cash_remained(self, currency, | ||
| USD_RATE=USD_RATE, EURO_RATE=EURO_RATE): | ||
| # def get_today_cash_remained(self, currency): | ||
| def get_today_cash_remained(self, currency, usd_rate=USD_RATE, euro_rate=EURO_RATE): |
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.
По условию технического задания, метод et_today_cash_remained(self, usd_rate=USD_RATE, euro_rate=EURO_RATE), должен принимать единственный параметр currency.
No description provided.