Skip to content

HW2_Trofimov #10

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

michtrofimov
Copy link

No description provided.

Copy link
Member

@nvaulin nvaulin left a comment

Choose a reason for hiding this comment

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

Хорошая работа! Все что нужно сделали.
Оставил в коде некоторые замечания по оформлению. Они не критические, просто обратите на них внимание на будущее. Спасибо за хорошие сообщения коммитов!

Рад что вы в итоге разобрались с пулл-реквестами в репозитории Мичила :)

Баллы:

К сожалению, не хватает все таки хотя бы небольшого описания в README того функционала, который сделала ваша команда: -0.4 балла.

  • За каждую функцию: 1.6 * 5 = 8
  • За README 0.6 + 1 доп. = 1.6
  • За наличие всех форков и пулл-реквестов - 1 балл

Итого: 10.6

Мерджить не надо, но стоит скинуть показать комментарии всем участникам команды

Молодцы!

Copy link
Member

Choose a reason for hiding this comment

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

А это тут что за зверь прокрался? 😁
Это какой то файл настроек папок ОС. Иногда встречаю его даже в тулах выложенных на GitHub. В общем следите за тем что добавляете в git, из-за таких как он поэтому и не стоит делать git add прям всей папки без разбора. Лучше поименно.

Copy link
Member

Choose a reason for hiding this comment

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

Это про файл .DS_Store

Comment on lines +2 to +9
calc_input = input()
calc_input_lst = calc_input.split()

operation = calc_input_lst[1]
number_1 = calc_input_lst[0]
number_1 = float(number_1)
number_2 = calc_input_lst[2]
number_2 = float(number_2)
Copy link
Member

Choose a reason for hiding this comment

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

В целом код рабочий, хоть и не очень питонячий. Особенно переменные типа calc_input и выглядят calc_input_lst громоздко (хотя нейминг по факту ок!). Это ничего страшного, мы с вами еще научимся опитонячивать наш код. Например, тут это можно было бы сделать так:

Suggested change
calc_input = input()
calc_input_lst = calc_input.split()
operation = calc_input_lst[1]
number_1 = calc_input_lst[0]
number_1 = float(number_1)
number_2 = calc_input_lst[2]
number_2 = float(number_2)
num1, operator, num2 = input().split()
num1, num2 = float(num1), float(num2)

Copy link
Author

Choose a reason for hiding this comment

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

трюк со сплитом инпута в три переменные хорош...

calc_input_lst = calc_input.split()

operation = calc_input_lst[1]
number_1 = calc_input_lst[0]
Copy link
Member

Choose a reason for hiding this comment

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

number_1 и number_2 хорошие названия, молодцы! Главное что не a и b или не x и y. Я бы сократил, например, до num1 и num2. Но ваш вариант тоже ок.

return print(res)


def multiplication(number_1, number_2):
Copy link
Member

Choose a reason for hiding this comment

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

Лучше было бы назвать функции глаголами: multuply, add, subtract, divide.

Comment on lines +23 to +24
res = number_1 * number_2
return res
Copy link
Member

Choose a reason for hiding this comment

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

Можно было бы это немного сократить:

Suggested change
res = number_1 * number_2
return res
return number_1 * number_2

Но это опять же про питонячесть, что понятие не шибко уловимое.

Comment on lines +30 to +31


Copy link
Member

Choose a reason for hiding this comment

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

Не знаю автоформатировали ли вы код, но отдельное спасибо за все пробелы и пустые строки где это нужно!

return res


main()
Copy link
Member

Choose a reason for hiding this comment

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

Такие вещи еще зачастую пишут через:

if __name__ == '__main__':
    main()

На следующей лекции как раз разберем зачем это нужно

res = multiplication(number_1, number_2)
elif operation == "/":
res = division(number_1, number_2)
return print(res)
Copy link
Member

Choose a reason for hiding this comment

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

А вот это после лекции 3 мы знаем что вернет нам None. Потому что print выдает None и мы его перехватим в return. Это я кажется в ТЗ не очень хорошо прописал по поводу результата, но всё-таки либо print(res), либо return res

Copy link
Author

Choose a reason for hiding this comment

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

цэ убийственный кринж... каюсь..

@@ -0,0 +1,12 @@
# Calculator
Yep. Calculator.
Copy link
Member

Choose a reason for hiding this comment

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

Yep

Comment on lines +8 to +12
- Ilia Popov ('multiplication' function)
- Julia Nechaeva ('addition' function)
- Ekaterina Shitik ('subtraction' function)
- Ricardo Milos / Pavel Grobushkin ('division' function)
- Michil Trofimov ('main' function)
Copy link
Member

Choose a reason for hiding this comment

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

Круто что вы перечислили обязанности каждого члена команды 👍
Еще бы подписать тут кто тимлид.
И не хватает самого хотя бы небольшого описания вашей мини-программы magnificent piece of software engineering.

Copy link
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.

5 participants