6
\$\begingroup\$

I'm relatively new to Python. I just want some constructive feedback on how to improve my code efficiency, style, error handling, etc.

In this case, I've programmed the Ackermann function, but it won't evaluate well since it's built to handle the recursions relatively poorly - I'll have to fix that in the future. Any tips?

Also, if there are any areas of exploitation in the errors, please let me know too.

For reference, I'm programming this on Ubuntu 16.04 LTS and the language is Python 2.7.12

# Make sure the collected values are of the appropriate forms
def collection ():
    while True:
        # Collect raw_input for m_val and n_val
        m_val = raw_input ("Please enter a nonnegative integer (m) : ")
        n_val = raw_input ("Please enter another nonnegative integer (n): ")

        # Make sure input is acceptable
        try:
            m_val = float(m_val)
            n_val = float(n_val)

            # m_val needs to be a nonnegative integer
            # Check equivalence to 0 first
            if m_val == 0:
                pass
            # Then check if the value is nonintegral
            elif int(m_val) - m_val:
                print("\nYou entered a float for m!\n")
                continue
            # Then make sure the value is nonnegative
            elif m_val < 0:
                print("\nYou entered a negative value for m!\n")
                continue

            # n_val needs to be a nonnegative integer
            # Check equialence to 0 first
            if n_val == 0:
                pass
            # Then check if the value is nonintegral
            elif int (n_val) - n_val:
                print("\nYou enterd a float for n!\n")
                continue
            # Then make sure the value is nonnegative
            elif n_val < 0:
                print("\nYou entered a negative value for n!\n")
                continue

            # If the code makes it this far, it should be ready for return
            return int(m_val), int(n_val)

        except ValueError:
            print("\nPlease enter numerical values for m and n.\n")

def ackermann(m,n):
    if m == 0:
        return n + 1
    elif m > 0 and n == 0:
        return ackermann(m-1,1)
    elif m > 0 and n > 0:
        return ackermann (m-1, ackermann(m,n-1))
    else:
        print "The Value Doesn't Go into the Domain!"

(m,n) = collection()
print ( ackermann(m,n) )
\$\endgroup\$
4
\$\begingroup\$
def ackermann(m,n):
    ...
    else:
        print "The Value Doesn't Go into the Domain!"

Printing the error message to standard output looks like a bad design choice (if you code is run automatically and no one is checking the output, it would just silently return None. I don't think you want it). It would more reasonable to throw an exception in this case (I'd throw an instance of the ValueError) because a situation is exceptional (the input is invalid and there's no meaningful way for this function to handle it). Moreover, one function should be responsible for one thing. Your code computes the value of the ackermann's function and does the logging at the same time. It's another reason to handle invalid inputs using exceptions.

You can also simplify the code in the collection function by parsing the input as an integer. There's no point in parsing it as a float and then checking if it's an integer. You could rely on a standard int function to do this job for you. This way, it would be enough to check that both numbers are non-negative.

Now let's talk about variable naming:

  1. collection is a strange name for a function. It's name doesn't give any clue about what it actually does. Even something simple and generic like read_input would be better.

  2. It's a bad practice to use one variable for two different things. n_val and m_val stand for the user's input as a string and then they turn into integers. I'd rather create two separate variables for each of them (one for the input and the other one for it's integral value).

You use of whitespace is inconsistent. For instance, there's a space after the name of the collection function, but there's none after the name of the ackermann function. Whatever style you choose, you should always be consistent with it (according to the PEP standard, there should be no space after the function name, but there should be one after the comma. I would stick to it unless I have compelling reasons not to).

It's also a good practice to write doc comments for all your functions and classes. The comments inside the code should be more about telling why the code does what it does or why a specific design decision was made. Redundant comments that just tell what the code does create noise and actually make it less readable. For instance, this comment is self-evident:

# Check equivalence to 0 first
if m_val == 0:
    pass

You can just remove it (and all other similar comments). Moreover, if you need to make a comment about what the piece of code does (like "# Then check if the value is nonintegral"), it's a hint that it should probably be implemented as another function with a descriptive name. You should strive for a self-documenting code.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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