12
\$\begingroup\$

The program was to turn an integer into exact change and uses a bunch of if statements.

Are there steps a beginner like me can do to remove some or is it just how it will be? I can't find anything on it. At least I'm not sure as I'm still in the basic course.

The program is turned in and was fine and this would be for my personal use. I know I may learn this in my classes in time, but I find more direct answers to help more.

change = int(input('')) # receive input from the user
# # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # #
#                                                                               #
# This code it so provide exact change using the least amount of coins possible #
#                         COMPUTER SCIENCE 161A                                 #
#                                                                               #
# # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # #
if change > 0:                          # checks if there is change to provide coins
    if change >= 100:                   #checks if the amount is greater or equal to 100
        if (change // 100) != 0:        # this is to check if there is change at this point
            if (change // 100) > 1:     # provide the plural if more than 1
                print('{} Dollars'.format(change // 100))
                change = (change % 100)
            else:
                print('{} Dollar'.format(change // 100)) # prints the dollar amount without a float
                change = (change % 100) # this leaves the remainder after calculating the amount of change
        else:                           # if there is no change do this
            change = (change % 100)     # if there no change here there will be no output
    if change <= 99:                    #checks if the amount is lower or equal to 99
        if (change // 25) != 0:         # this is to check if there is change at this point
            if (change // 25) > 1:
                print('{} Quarters'.format(change // 25))
                change = (change % 25)
            else:
                print('{} Quarter'.format(change // 25))
                change = (change % 25)
        else:
            change = (change % 25)
    if change <= 24:                    #checks if the amount is lower or equal to 24
        if (change // 10) != 0:         # this is to check if there is change at this point
            if (change // 10) > 1:
                print('{} Dimes'.format(change // 10))
                change = (change % 10)
            else:
                print('{} Dime'.format(change // 10))
                change = (change % 10)
        else:
            change = (change % 10)
    if change <= 9:                     #checks if the amount is lower or equal to 9
        if (change // 5) != 0:          # this is to check if there is change at this point
            if (change // 5) > 1:
                print('{} Nickels'.format(change // 5))
                change = (change % 5)
            else:
                print('{} Nickel'.format(change // 5))
                change = (change % 5)
        else:
            change = (change % 5)
    if change <= 4:                     #checks if the amount is lower or equal to 4
        if (change // 1) != 0:          # this is to check if there is change at this point
            if (change // 1) > 1:
                print('{} Pennies'.format(change // 1))
            else:
                print('{} Penny'.format(change // 1))
else:
    print('No change')                  # tells the user to fuck off if there is nothing to convert
\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to CodeReview@SE! Seeing you don't follow the Style Guide for Python Code, if you are fairly new to Python, tag your question beginner. (You may want to have a look at reviews for Python stabs at the Change Making Problem.) \$\endgroup\$ – greybeard Feb 27 at 7:02
  • \$\begingroup\$ @greybeard, You have provided excellent advice! Brought me right to what I was looking for. Thank you very much. \$\endgroup\$ – Traveler Feb 27 at 9:55
  • 1
    \$\begingroup\$ @Traveler Hi! You did a good job posting your first question and yes, your post is a good fit here :) I edited your post to space the text a bit more and also, I changed your title. We want titles that reflect what the code does, not what we want in a review. I hope you will follow those guidelines in your next posts and welcome to the world of programming :) \$\endgroup\$ – IEatBagels Feb 27 at 20:47
  • 1
    \$\begingroup\$ @IEatBagels I was just looking over the corrections you made and was looking to thank you, since you are right here now, thank you. I will do my best in the future to use the correct formatting, I'm sure the block of text was not easy to read, and the simpler title is much more appealing. \$\endgroup\$ – Traveler Feb 27 at 20:53
  • \$\begingroup\$ @Traveler You're most welcome! \$\endgroup\$ – IEatBagels Feb 27 at 20:55
17
\$\begingroup\$

Firstly, I think your overall approach is sensible. You work your way through the coin types, from highest to lowest, pulling out as many coins of that type as possible, and passing the remainder to the next stage. So, logically, you're on the right track.

But as you've probably noticed, there's a lot of duplication in your code. You need to think about what you're doing the same for each coin type, and pull that logic into its own function. Which will yield something like the following:

def calculate_coins_of_denomination(change, denom):
    """
    Returns a tuple where the first value is the number of coins of the given denomination,
    and the second is the remainder

    Examples:

    calculate_coins_of_denomination(105, 100) => (1, 5)
    calculate_coins_of_denomination(100, 25) => (4, 0)
    """
    num_coins = change // denom
    return (num_coins, change - (num_coins * denom))

Once you've got this function in place, you can then call it repeatedly with each coin type, passing just the remainder to each successive function call:

(num_dollars, change) = calculate_coins_of_denomination(change, 100)
(num_quarters, change) = calculate_coins_of_denomination(change, 25)
(num_dimes, change) = calculate_coins_of_denomination(change, 10)
(num_nickels, change) = calculate_coins_of_denomination(change, 5)
(num_pennies, change) = calculate_coins_of_denomination(change, 1)

Now you've got the values for num_dollars etc, it's just a question of printing your output in the desired format.

As a final note, avoid using profanity or insults in code comments, even for little experimental programs like these. The brief satisfaction that it yields does not justify the potential cost in terms of looking unprofessional or developing bad habits.

| improve this answer | |
\$\endgroup\$
  • 6
    \$\begingroup\$ Just a note: putting explicit parentheses around num_X, change is not necessary, and I would argue that they don't improve the readability here. \$\endgroup\$ – AlexV Feb 27 at 11:41
  • 6
    \$\begingroup\$ Also perhaps consider using the divmod function. Won't be available in every language, but it'll clean up your relatively ugly return line. \$\endgroup\$ – acdr Feb 27 at 13:49
  • 2
    \$\begingroup\$ Keep in mind that these repeated calls to calculate_coins_of_denomination could also be rolled into a loop, like num_coins = {}, for (denom, name) in [(100, "dollars"), (25, "quarters"), ... (1, "pennies")]: num_coins[name], change = calculate_coins_of_denomination(change, denom) \$\endgroup\$ – acdr Feb 27 at 13:52
  • \$\begingroup\$ @acdr Luckily other languages are not a major concern in that case and Python has divmod ;-) \$\endgroup\$ – AlexV Feb 27 at 14:43
  • 5
    \$\begingroup\$ In addition to avoiding profanity in comments, it is usually better to avoid comments altogether if it is obvious what the code is doing. I don't need a comment to tell me what print('No change') or if change <= 4: does. I can tell that by reading the code. You only need a comment if it may not be clear why it was done that way. I.e. don't use comments to explain what the code does, just why it's there. \$\endgroup\$ – Seth R Feb 27 at 17:10
15
\$\begingroup\$

I noticed that you have a ton of comments in your code. Now don't get me wrong, comments are great, but there is such a thing as too many. Good code will be self-documenting, which means that you should be able to understand it well even without comments. Some of these comments are extremely unnecessary such as

 if change <= 9:                     #checks if the amount is lower or equal to 9

Any programmer can tell what this is doing without the comment. You don't need it.

Other than that I'd recommend more modularity. As others have pointed out there's a lot of repetition in your code. You can fix this by using functions.

| improve this answer | |
\$\endgroup\$
  • 5
    \$\begingroup\$ +1 on this. My code tends to have a lot of comments - and none of them look like the OP's. Comments should always say why the code is doing what it's doing. As your answer says, what it's doing can be seen from the code, so the comments shouldn't mention that. \$\endgroup\$ – Graham Feb 27 at 16:54
  • 1
    \$\begingroup\$ I understand the commenting is a bit excessive, for now, I'm doing that just to tell myself and nothing more. Just a way for me to put things in my head. \$\endgroup\$ – Traveler Feb 27 at 20:30
9
\$\begingroup\$

Python is a somewhat terse language.
It has what it calls In-place Operators. It is unusual to see them not used: change the likes of
change = (change % whatever) to change %= whatever

There are places where (almost) the same code reoccurs. There is procedural abstraction: once you give a procedure a (telling) name, you can use it in a terse way, time and again.
If differences in value are all that makes code ranges just almost the same, introduce a parameter.

There are places where the same code appears once in every path in a nested conditional statement. Trivial fix if at the beginning or end: move before or after that statement.

    nickels = change // 5
    if nickels != 0:  # this is to check if there is change at this point
        if nickels > 1:
            print('{} Nickels'.format(nickels))
        else:
            print('{} Nickel'.format(nickels))
    change %= 5

(Well, "the" pythonic way may be coins, change = divmod(change, denominations[d].)
(Having been raised with %-formatting, if that, I don't have preferences re. string.format() vs. the misleadingly named formatted string literals.)

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ That is great advice you provided here! I cannot recall actually being taught to do things that way yet, though I could have easily missed it. I will do my best to remember this for the future. \$\endgroup\$ – Traveler Feb 27 at 20:34
6
\$\begingroup\$

A small trick I can give you is to do your "exit conditions" first.

For example, you have

if change > 0 :
    ...

The issue is that you then have the majority of your code indented. If you invert the condition and return, you can do this :

if change <= 0:
    print('No change')
    return
if change >= 100:
    ...

This would give you the advantage of having less indentation in your code. Indentation is really really bad for cognitive complexity.

Failing fast also lets your code execute faster when it fails. In your case, this won't really apply, but if you were doing expensive calls in your conditions, you would want to fail as early as possible.

Always check what inverting your conditions results in. In some cases, it can make the code a lot easier to read.

| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ @Hugu-Viallon I really like this as it has not crossed my mind before to have an exit from the start and I can clearly understand why it would be great to use. Thank you very much for this! \$\endgroup\$ – Traveler Feb 27 at 20:37
5
\$\begingroup\$

The key to programs like this (or, to be honest, to any program) is to think carefully about abstractions. When approaching any programming problem, the first step is to choose the level of abstraction at which you want to work.

In this case, the obvious abstraction is the notion of "coins" to represent nickels, dimes, quarters, etc. That tells you two things:

  • Data representation: You'll want some abstract way of representing "coins" as data.
  • Functions/methods: Any functions or routines that you write should use this abstract "coin" representation when performing any calculations.

There are many choices for how you actually do this. In his answer, Pete presented a good way to do the second bullet: a generic function that will work for any denomination of coin. Personally, I would do things in a different order and start by thinking about the first: how to represent a coin.

One way, of course, is to use the full blown object-oriented approach in all its glory: create a "coin" class and write methods to do all the manipulation (to include printing the output). This is the approach emphasized in most computer programming classes, and there's nothing wrong with it except that it may be a bit overkill for such a simple application.

Another approach is to use simpler data constructs, such as lists or dictionaries, to represent coins. You can then loop over the elements in the list to give you the correct change. An example of that approach using dictionaries is:

coinTypes = {'Dollars':100, 'Quarters':25, 'Dimes':10, 'Nickels':5, 'Pennies':1}

money = int(input(''))  

for name,denom in coinTypes.items():
    ncoins, money = divmod(money, denom)
    print('{}: {}'.format(name, ncoins))

Minor variations could use lists instead of dictionaries, or use a separate list/dictionary to keep track of the number of coins for future use. Again, there's no "right" design for the program, but (in my opinion) there is a right way to think about it, and that is to start by thinking about the abstractions you want to use.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ I like the way this looks and helps me understand how abstraction works in Python. Would a second dictionary be needed if I needed to output singular or plural as in the case here, it was asked to do so in my code? I believe I actually left that part out :/ \$\endgroup\$ – Traveler Feb 27 at 20:46
  • 1
    \$\begingroup\$ Good question; there are different approaches. For singular/plural, you might have to go to a separate structure to store the singular/plural names, or make a more complex dictionary (so something like "coinTypes = {100: ['dollar', 'dollars'], ..." or such). But the important thing is to have the definitions stored in the data structure(s), so that you just have to change the values in order to switch systems (pounds and shillings, for example, or if you can't use dimes). \$\endgroup\$ – Richter65 Feb 27 at 21:23
  • 1
    \$\begingroup\$ Another important thing is not to have duplicate information that can become inconsistent. If you make a change, design it so you need to make the change only in one place. So, for example, you don't want to do something like manually write two dictionaries with the same keys. That could cause problems if you made a change to one, but not the other. \$\endgroup\$ – Richter65 Feb 27 at 21:25
  • 1
    \$\begingroup\$ That seems like a very clean approach, I still don't understand fully the capabilities of dictionaries and the likes but can see they are very great at what they can do. I do understand what you mean by using duplicates and hopefully can grasp the entire function tree to use this for the better now! \$\endgroup\$ – Traveler Feb 28 at 5:16
  • 1
    \$\begingroup\$ I'd also separate the output from the computation. I'd fill in a dictionary in the cycle with the coin amounts and then iterate over it to print the results. Only this second cycle will need the coin names and need to deal with pluralization. \$\endgroup\$ – Rad80 Feb 28 at 13:37

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.