9
\$\begingroup\$

I created a basic program that allows a user to continuously input values until he or she is satisfied; furthering, the program outputs the mean, median, and range. Then, the code gives you the option to run it again or exit.

#Mean, median, and range
from clear import *
def main():
    clear()
    list_one = []
    while True:
        q = input('Type a number or type stop at any time: ').lower()
        if q.isnumeric():
            list_one.append(int(q))
            print('''
Great choice! Choose again or type stop the sequence.''')
        elif q == 'stop':
            list_one.sort(key=int)
            print('''
And the values are...''')

            def mean():
                return 'Mean: ' + str(sum(list_one)/len(list_one))

            def median():
                length = len(list_one)
                if length % 2 == 0:
                    return  'Median: ' + str(sum(list_one[length//2-1:length//2+1])/2)
                else:
                    return 'Median: ' + str(list_one[length//2])

            def range_():
                return 'Range: ' + str(list_one[-1] - list_one[0])

            print(mean())
            print(median())
            print(range_())
            break

        else:
            print('''
That\'s not a number or stop! Try again brudda man.
''')

def end():
    while True:
        q = input('Type anything and and press enter to repeat or press only \'Enter\' to exit. ')
        if len(q) >= 1:
            main()
            break
        else:
            exit()
main()

while True:
    q = input('''
Type anything and and press enter to repeat or press only \'Enter\' to exit. ''')
    if len(q) >= 1:
        main()
    else:
        clear()
        exit()

Additionally, the "clear" library that I imported is a custom library of my making which only prints out a ton of blank lines so the script "clears" and outputs are then at the bottom. If there is a better way of doing this please let me know.

I want to get familiar with what is "right" or "wrong" in terms of programming with Python. I would like my work to look professional and practice more respected coding ethics, so please be as harsh as you wish.

\$\endgroup\$
1
  • 3
    \$\begingroup\$ You need to include your external custom libraries if you have them. We need COMPLETE CODE including any additional code libraries you wrote, including clear. \$\endgroup\$ Commented Oct 18, 2018 at 19:56

4 Answers 4

10
\$\begingroup\$

Disclaimer: You asked me to be harsh, so I'm not holding back as much. I tried to though...

Welcome to Code Review! Your python code has been put through my scrutiny and I have some concerns and suggestions for improvement. Any criticisms and scrutiny are to be taken at their face value unless stated otherwise, and may include extremely harsh criticisms. I tried to avoid being overly harsh, but there are cases where I have to be harsh because of the severity of the mistakes or issues.


MISSING MODULES AND CODE: clear

You state you wrote your own code for clear. Because I don't have your clear() code I can't review it, and to make sure things work I need to yank it out of the code for review purposes. So, clear() won't exist in the final code examples.



The missing code for clear aside, this is the review I have written so far:

from clear import *: BAD FORM!

Wildcard imports are bad form! PEP8, the Style Guide for Python, explicitly discourages using wildcard imports.

To quote PEP8:

Wildcard imports (from import *) should be avoided, as they make it unclear which names are present in the namespace, confusing both readers and many automated tools. There is one defensible use case for a wildcard import, which is to republish an internal interface as part of a public API (for example, overwriting a pure Python implementation of an interface with the definitions from an optional accelerator module and exactly which definitions will be overwritten isn't known in advance).

You are not taking a private internal interface and republishing it for public API, so that doesn't apply. Replace any functions you're using from clear.* with clear.FUNCTION instead. This is so we know what module the specific bit(s) work with.

Alternatively, you can import the specific function with:

from clear import clear

... which would let you do clear() calls like you have in your code. I have gone with this for the code example at the end.


Bad form: declaring modules inside your main() function

This isn't C, C++, or Java. You don't need to declare your functions within main(). In fact, unless you are working with classes and defining functions for a class, you should not be declaring your functions within a class. Take your mean, median, and range_ functions and declare them individually outside the main class. Your general code structure would then look like this:

def mean():
    ...

def median():
    ...

def range_():
    ...

def main():
    ...

This is completely acceptable and in fact the proper way to do these types of declarations.


Unused Function: end()

You create the function end but don't use it. Either leave it out entirely because its code is already written into the code that executes after main, or replace the code after the main() call with end(). However, I would suggest that you do the second of these, rather than call (end) just to re-call main() later.


Use escape characters instead of triple-apostrophe strings when working with New Lines

You have this type of print statement at least twice:

    print('''
Great choice! Choose again or type stop the sequence.''')

Functionally, this works, but from a code readability perspective, it's ugly. Replace this with escape-charactered strings instead, like:

print("\nGreat choice! Chose again or type stop the sequence.")

This reads better to those of us doing the reviews.


Use string formatting rather than appending strings to other strings

Here's an example of what I'm talking about. You do this in many different places in the code:

return 'Mean: ' + str(sum(list_one)/len(list_one))

This works, but... string appending is not the nicest thing in the world. The proper way to do this nowadays, because you'll undoubtedly in the future have longer strings with multiple things needing inserted into it instead of just one thing at the end, is to use a format string. This'd be such as this:

return 'Mean: {}'.format(sum(list_one) / len(list_one))

This also reads slightly nicer because string casting just takes up extra characters and space. Also you'll note that with this I added some spaces around that / - this makes math more readable, actually, to those of us doing reviews.


Use one print line instead of multiple

print, when combined with strings and line endings and format capable strings can actually post many things at once!

So let's take my last suggestion and turn your print calls for mean, median, and range into one statement:

print("{}\n{}\n{}".format(mean(), median(), range_()))

We get to save some typing here, too, as well.


Consider including #! (shebang) syntax at the first line

This way, we can call your Python code directly in command lines such as the Linux shell with ./file.py and execute the code that way (if execute bit is set). This is not a requirement, but it's nice to have so I don't have to type python3 ./filename.py or such to run your script.


I apologize if I was overly harsh, but you said not to hold back, and I gave you a warning early on. I tried to be as non-harsh as I could.

With all my suggestions above, you get something like this:

#Mean, median, and range

def mean():
    return 'Mean: {}'.format(sum(list_one) / len(list_one))


def median():
    length = len(list_one)
    if length % 2 == 0:
        return 'Median: {}'.format(sum(list_one[length // 2 - 1:length // 2 + 1]) / 2)
    else:
        return 'Median: {}'.format(list_one[length // 2])


def range_():
    return 'Range: {}'.format(list_one[-1] - list_one[0])


def main():
    list_one = []
    while True:
        q = input('Type a number or type stop at any time: ').lower()
        if q.isnumeric():
            list_one.append(int(q))
            print("\nGreat choice! Choose again or type stop the sequence.")
        elif q == 'stop':
            list_one.sort(key=int)
            print("\nAnd the values are...")

            print("{}\n{}\n{}".format(mean(), median(), range_()))
            break

        else:
            print("\nThat\'s not a number or stop! Try again brudda man.")

main()

while True:
    q = input("\nType anything and and press enter to repeat or press only \'Enter\' to exit. ")
    if len(q) >= 1:
        main()
    else:
        exit()

However, we need the code for your clear module to be able to do a full and complete review, so this review is only partial until such time you provide us with that code. Note that I yanked all clear() references in the interim as well.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Functions are not declared inside main() in any of C, C++ or Java. Here main is not a class. And it's correct to declare a function inside another function in Python (though useless here). "While Python typically calls main()": no, Python calls nothing if you don't call explicitly main() in the program. The if __name__ == "__main__": trick is only useful for programs that can be used as modules (not often my use case, so I'm indeed happy with main()). The shebang is only necessary if you plan to use/distribute your program on Unix-like systems. Why would it be better to print only once? \$\endgroup\$
    – user179210
    Commented Oct 19, 2018 at 8:10
  • \$\begingroup\$ Too much information for me to comment on everything, but overall I highly appreciate your time and will probably read it a few times over, thanks much! \$\endgroup\$
    – Brandon
    Commented Oct 22, 2018 at 0:52
13
\$\begingroup\$

On top of Thomas Ward answer, here is a short and important comment:

More beautiful functions

Your functions are performing some mathematical computations and handling the formatting to show the result to the 2 users. These correspond to 2 concerns that could be separated.

Also, your functions rely on the list_one global variable. It would be clearer to have this provided to the functions using an argument.

You'd have something like:

def mean(lst):
    return sum(lst) / len(lst)

def range_(lst):
    return lst[-1] - lst[0]

def median(lst):
    length = len(lst)
    if length % 2 == 0:
        return sum(lst[length//2-1:length//2+1]) / 2
    else:
        return lst[length//2]


...
            print('Mean: ' + str(mean(list_one)))
            print('Median: ' + str(median(list_one)))
            print('Range: ' + str(range_(list_one)))

These are easier to understand and to reason about. They are also easier to test. This is an interesting exercise that you can try to practice.

Small improvements in median

The expression length//2 is used in multiple places which makes the code tedious to read and less efficient than it could be. Using a temporary variable makes things clearer:

def median(lst):
    length = len(lst)
    mid = length//2
    if length % 2 == 0:
        return sum(lst[mid-1:mid+1]) / 2
    else:
        return lst[mid]

From here, I think it's pretty good already. Let's see what can be still be done for the sake of learning new techniques.

When you want to compute both the quotient and the remainder of the division, you can use "/" and "%" just like you did. Another option is to use the divmod builtin which returns both directly:

def median(lst):
    length = len(lst)
    mid, rem = divmod(length, 2)
    if rem == 0:
        return sum(lst[mid-1:mid+1]) / 2
    else:
        return lst[mid]

The operation sum(lst[mid-1:mid+1]) / 2 looks a lot like things we've just somewhere else: we compute the sum of a list and we divide by the number of elements which is happens to be 2.

We could reuse our mean function here.

def median(lst):
    length = len(lst)
    mid, rem = divmod(length, 2)
    if rem == 0:
        return mean(lst[mid-1:mid+1])
    else:
        return lst[mid]

To go further, we could use the ternary operator to have a single return value. We could also use the fact that in a boolean context, non-zero integers are equivalent to True and zero is equivalent to false. This would lead to the following code which is, from my point of view, not really better:

def median(lst):
    length = len(lst)
    mid, rem = divmod(length, 2)
    return lst[mid] if rem else mean(lst[mid-1:mid+1])
\$\endgroup\$
4
  • \$\begingroup\$ Nice catch, I did a fast review and caught the big problems rather than nitpicking in this case. +1'd! \$\endgroup\$ Commented Oct 18, 2018 at 20:16
  • 2
    \$\begingroup\$ An "even more beautiful" solution would be from statistics import mean, median; and range_ would be max(lst) - min(lst) to avoid the need for a sorted list. \$\endgroup\$ Commented Oct 19, 2018 at 15:06
  • \$\begingroup\$ @MathiasEttinger that's an excellent comment. I answered as if the question was tagged "reinventing-the-wheel" but I completely missed the was that sorting the input was a requirement. \$\endgroup\$
    – SylvainD
    Commented Oct 19, 2018 at 15:18
  • \$\begingroup\$ I like the more efficient method and thank you for your time. \$\endgroup\$
    – Brandon
    Commented Oct 22, 2018 at 0:53
4
\$\begingroup\$

In addition to Josay's points about the functions mean(), median() and range_(), it's worth checking that the list is not empty (either within the functions or before calling them). As it is there is nothing to prevent the user entering stop¹ immediately. If you try to process an empty list, mean() will cause a ZeroDivisionError, range_() will cause an IndexError, and median() will give 0 (I think None would be more appropriate here).

¹ aside: consider using if q.lower()=='stop' for a case-insensitive test

\$\endgroup\$
1
  • \$\begingroup\$ Ah I see, thank you for the tip I'll make sure to think ahead about that kind of stuff in the future. \$\endgroup\$
    – Brandon
    Commented Oct 22, 2018 at 0:50
0
\$\begingroup\$

About the external library you want for a better-efficient way to clear screen.

The solution from this, is coming from an external Library, called "os".

Finally, there are 2 ways to write it. The 2nd is the most correct.

So, the code will be: (1)

import os

os.system('cls')

That applies to only Windows users.

Or, 2nd way (For Windows and Mac users)

import os


def clear():

    #For Windows
    if os.name == 'n' :
        - = os.system('cls')


    #For MacOS and Linux (The os.name here is 'posix'

     else:
         - = os.system('clear')


 #Now, when you want to clear your screen, you do:

clear()
\$\endgroup\$
1
  • \$\begingroup\$ Thank you for the alternative way to clear the console, I will keep it in mind! \$\endgroup\$
    – Brandon
    Commented Oct 22, 2018 at 0:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.