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

AmirG - All done without the Bonus :( #15

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

Conversation

amirg33
Copy link

@amirg33 amirg33 commented Oct 12, 2023

No description provided.

@sh-ih
Copy link

sh-ih commented Oct 16, 2023

Amir,
Great job on this lab! ⭐

Some comments:

On question 2, your function passes the test but there is a case where setting largest=0 would create some issues: when your array has only negative numbers. If you run: greatest([-1,-5,-800]) you’ll get 0 as result, instead of -1.
A way to deal with this cases is to set largest=+inf. In Python, +inf is an unbounded upper value.
Another option would be to set largest=arr[0]

On question 5, you could simplify your code by calling the functions you created on the previous two questions:

def oper_all(arr, oper):
    if oper == "+":
        return sum_all(arr)
    elif oper == "*":
        return mult_all(arr)

On Question 9, you created an empty set but named you variable as empty_list. The best practice when naming things is to be as precise as possible, because later on, you or another person using your code may see the name empy_list and think the data type is list but then it turns out it was always a set.

And on Question 10, also about naming: all your variables are named with lowercase words, except Upper. Here you are only working with a few variables and it may not be a problem, but soon you’ll start working with more complicated code and those naming exceptions start getting difficult to remember or apply and can create errors you’ll have to debug.

It would be best if you start to get used to naming best practices since the start: lowercase for function names and variables, with words separated by underscores as necessary to improve readability is Python’s naming convention (source: PEP8 naming conventions)

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