11
\$\begingroup\$

DESCRIPTION:
[Inspired by Chandler's GameOfCups with Joey in "Friends"]. Program gets 5-digit zipcode from user. (Assume user won't make a mistake, and will enter exactly 5 digits). Program awards points based on a series of rules, and reports the total points earned at the end. The 8 rules are embedded as comments in the code. For each rule, besides adding points (or not) to the total, rule displays "Rule _ got _ points, so total is now _" (It prints this even if rule added 0 points to total).

"""
RULES
    +5 when first and last digit match
    +6 when second digit is twice the first AND third digit is greater than second or fourth digit
    +7 if any 7 is in the zipcode
    +8 when there's no "13" in MIDDLE the zipcode
    +9 when all three middle digits match
    +10 when third and fourth digits match
    +11 when zipcode is palindrome (12121 == 12121, while 12345 != 54321)
"""

Here is my solution to the challenge above:

zipcode = input("Enter your zipcode: ")
total_points = 0

#Rule 1

points = 5 if zipcode[0] == zipcode[-1] else 0
total_points += points
print(f"Rule 1 got {points} points, so total is now {total_points}")

#Rule 2

points = 6 if (int(zipcode[1]) * 2) > int(zipcode[0]) and (int(zipcode[2]) > int(zipcode[1]) or int(zipcode[2]) > int(zipcode[3])) else 0
total_points += points
print(f"Rule 2 got {points} points, so total is now {total_points}")

#Rule 3

points = 7 if "7" in zipcode else 0
total_points += points
print(f"Rule 3 got {points} points, so total is now {total_points}")

#Rule 4

points = 8 if "13" not in zipcode[1:-1] else 0
total_points += points
print(f"Rule 4 got {points} points, so total is now {total_points}")

#Rule 5

points = 9 if zipcode[1] == zipcode[2] == zipcode[3] else 0
total_points += points
print(f"Rule 5 got {points} points, so total is now {total_points}")

#Rule 6

points = 10 if zipcode[2] == zipcode[3] else 0
total_points += points
print(f"Rule 6 got {points} points, so total is now {total_points}")

#Rule 7

points = 11 if zipcode == reversed(zipcode) else 0
total_points += points
print(f"Rule 7 got {points} points, so total is now {total_points}")

print(f"{zipcode} got {total_points} points!")

I feel like there is a much better way to do this. The print statements are repetitious, and reassigning points each time I check the zip code feels weird. Any suggestions are helpful and appreciated.

\$\endgroup\$
13
\$\begingroup\$

Your code can be simplified using a simple loop, eliminating most of the duplicated code:

def game_of_cups(zipcode, rules):

    total_points = 0

    for num, rule in enumerate(rules, 1):
        rule_passes = rule(zipcode)
        points = num + 4 if rule_passes else 0
        total_points += points
        print(f"Rule {num} got {points} points, so total is now {total_points}")

    print(f"{zipcode} got {total_points} points!")

You just need the appropriate rule functions, like:

def rule1(zipcode):
    return zipcode[0] == zipcode[-1]

def rule2(zipcode):
    a, b, c, d, e = map(int, zipcode)
    return b * 2 > a and c > min(b, d)

... etc ...

And then a list of rules to pass to the game:

rules = [ rule1, rule2, rule3, rule4, rule5, rule6, rule7 ]

Feel free to name the functions more appropriately; they don’t need to be named rule#.


Are you missing a rule? You said there were 8 rules.


Your implementation of rule#2 doesn’t match the comment description of rule #2. I think it should be b == a * 2, not b * 2 > a.

\$\endgroup\$
4
\$\begingroup\$

Just a modification on AJNeufeld's

def game_of_cups(zipcode, rules):
  for num, rule in enumerate(rules, 1):
    points = rule(zipcode)
    total_points += points
    ...

def rule1(zipcode):
  return 5 if (zipcode[0] == zipcode[-1]) else 0

def rule2(zipcode):
  a, b, c, d, e = map(int, zipcode)
  return 6 if (b == 2 * a and c > min(b, d)) else 0

...

As long as there aren't participation points awarded for any given rule, then this method works for testing to see if the is passed based on the fact that points were awarded.

rule_passes was never really used aside from adding the points.

Finally, this allows for more robust point system (and is just as hard coded as the former answer).

\$\endgroup\$
4
\$\begingroup\$

Personally, I think it's excessive to have a function for each of the rules. IMO, if you only need the function in a single context, you probably don't need to make a function out of it unless it's sufficiently complex to warrant one - and even then, make sure it's "private."

Because the values for the rule set start at 5 and simply increment in value from rule to rule, you can just iterate over a boolean list, like:

total = 0
for x, rule in enumerate(rules):
    score = (x+5) * rule
    total += score
    print(f"Rule {x+1} got {score} points, so total is {total}.")


As for that boolean list, you could do something like:

digits = list(map(int, zipcode))
rules = [
    zipcode[0] == zipcode[-1],
    digits[1] == 2*digits[0] and digits[2] > min(digits[1], digits[3]),
    "7" in zipcode,
    "13" not in zipcode[1:-1],
    min(digits[1:-1]) == max(digits[1:-1]),
    zipcode[2] == zipcode[3],
    zipcode == zipcode[::-1]
]

This way you have an list of zeroes and ones for multiplication in the previous loop. In particular, though, rule 2 looks pretty ugly when just slapped into the list here. If you want to make the list look a bit less ugly, maybe you could define rule1 through rule7 to equal each of those conditions and then construct the array that way.


I also stuck the code into an if __name__ == '__main__' for good practice, and called cupgame from within there. My final program looks like this:

"""
RULES
    +5 when first and last digit match
    +6 when second digit is twice the first AND third digit is greater than second or fourth digit
    +7 if any 7 is in the zipcode
    +8 when there's no "13" in MIDDLE the zipcode
    +9 when all three middle digits match
    +10 when third and fourth digits match
    +11 when zipcode is palindrome (12121 == 12121, while 12345 != 54321)
"""

def cupgame(zipcode):
    digits = list(map(int, zipcode))

    rules = [
        zipcode[0] == zipcode[-1],
        digits[1] == 2*digits[0] and digits[2] > min(digits[1], digits[3]),
        "7" in zipcode,
        "13" not in zipcode[1:-1],
        min(digits[1:-1]) == max(digits[1:-1]),
        zipcode[2] == zipcode[3],
        zipcode == zipcode[::-1]
    ]

    total = 0
    for x, rule in enumerate(rules):
        score = (x+5) * rule
        total += score
        print(f"Rule {x+1} got {score} points, so total is {total}.")

if __name__ == '__main__':
    zipcode = input("Enter your zipcode: ")
    cupgame(zipcode)

I do apologize if this is at all unclear - this is my first time reviewing code outside of a classroom, but hopefully this is of some use.

\$\endgroup\$
  • 1
    \$\begingroup\$ for x in range(len(rules)) is un-Pythonic, and inefficient since it requires an indexing operation rules[x] in the loop. Use for x, rule in enumerate(rules) instead. \$\endgroup\$ – AJNeufeld Sep 18 at 15:32
  • \$\begingroup\$ 1 in mid and 3 in mid and mid.index(3) > mid.index(1) will generate True for the zipcode "91939", but "13" in "91939" returns False, which means your implementation of rule #4 differs from the original. \$\endgroup\$ – AJNeufeld Sep 18 at 15:34
  • \$\begingroup\$ Rule #4 is still wrong. It fails for "91139", claiming there is no "13", but there is. \$\endgroup\$ – AJNeufeld Sep 18 at 15:55
  • 1
    \$\begingroup\$ @AJNeufeld Updated, hopefully that should do it on the silly bugs. xD \$\endgroup\$ – squid Sep 18 at 16:17
  • \$\begingroup\$ "make a function out of it unless it's sufficiently complex to warrant one" This kind of thinking leads to god classes and untestable 100 line functions. \$\endgroup\$ – Holli Sep 18 at 19:27
3
\$\begingroup\$

I think this is a beautiful opportunity to put them all in a dictionary.

"""
RULES
    +5 when first and last digit match
    +6 when second digit is twice the first AND third digit is greater than second or fourth digit
    +7 if any 7 is in the zipcode
    +8 when there's no "13" in MIDDLE the zipcode
    +9 when all three middle digits match
    +10 when third and fourth digits match
    +11 when zipcode is palindrome (12121 == 12121, while 12345 != 54321)
"""
def rule1(code):
    return code[0] == code[-1]

def rule2(code):
    return int(code[1]) == 2*int(code[0]) and int(code[2]) > int(code[1]) or int(code[2]) > int(code[3])

def rule3(code):
    return "7" in code

def rule4(code):
    return "13" not in code[1:-1]

def rule5(code):
    return len(set(code[1:-1])) == 1  # Only 1 unique symbol in this part of the string.

def rule6(code):
    return code[2] == code[3]

def rule7(code):
    return code == code[::-1]  # Checks for equality against it's own reverse.

rules = {
    rule1: 5,
    rule2: 6,
    rule3: 7,
    rule4: 8,
    rule5: 9,
    rule6: 10,
    rule7: 11,
}

code = input("Please enter code: ")
total = 0
for is_valid, worth in rules.items():
    if is_valid(code):
        total += worth
        print(f"{is_valid.__name__} got {worth} points, so total is now {total}")
    else:
        print(f"{is_valid.__name__} got 0 points, so total is now {total}")

First we define all our rules, in such way that they return a boolean value to show whether they apply or not. Then we put them in a dictionary, with the functions themselves as keys, and the worth as value - this allows us to have duplicate values in a hypothetical future. And yes, functions are perfectly valid dictionary keys, like all hashable objects.

Then we simply loop over the dictionary, adding up the values that belong to those functions that return true.

If printing wasn't necessary, it would be a lot easier to add the values up and print them right away with a Generator Expression, one of the tools responsible for making python as awesome as it is.:

print(f"Total value: {sum(worth for is_valid, worth in rules.items() if is_valid(code))}")
\$\endgroup\$
  • \$\begingroup\$ This omits the For each rule, besides adding points (or not) to the total, rule displays "Rule _ got _ points, so total is now _" requirement. \$\endgroup\$ – AJNeufeld Sep 18 at 15:11
  • \$\begingroup\$ You should also mention Python 3.6+ is required to guarantee rules are processed in the correct order. For Python 3.5 and earlier, an OrderedDict is required. \$\endgroup\$ – AJNeufeld Sep 18 at 15:14
  • \$\begingroup\$ A dictionary is actually unnecessary. A list of tuples would be better: rules = [(rule1, 5), (rule2, 6), ... (rule7, 11)]. Then the .items() call is unnecessary; simply use for is_valid, worth in rules. \$\endgroup\$ – AJNeufeld Sep 18 at 15:18
  • \$\begingroup\$ No, hash values are unique at least for Cython - they're based on the memory address of the code. Yes, tuples could work to. But dicts aren't that much worse. Dictionary order is completely irrelevant for this and many other purposes. I'll update for the printing. And Python 3.6 is required only for the f-strings. Python dicts are implemented with hash-tables, not equality tests. All keys are required to give a good representation when hash() is called on them. \$\endgroup\$ – Gloweye Sep 18 at 15:32
  • 1
    \$\begingroup\$ Still can't see a reason why functions as dictionary keys would be a bad idea. hash collissions is still the only way stuff goes wrong. Why exactly would that second identity check somehow mess up more? \$\endgroup\$ – Gloweye Sep 18 at 16:14
2
\$\begingroup\$

Your code is a straight-line solution to a straight-line problem. So congratulations! You are off to a much better start than you might feel.

Here are some obvious issues:

Why did you separate your docblock from your code? And why is the rationale not included in the docblock?

One of the ways to become a better programmer is to try to show empathy to "future you." You can do this by including more information than you might think is necessary about what you are doing and why you are doing it.

In this case, if you have written a program with these rules included, the by all means include the challenge itself in the program, as well as a link to the URL of the challenge, and the circumstances under which you are taking the challenge:

""" rule-of-cups.py

    Dave K. and I were drinking last night (17 Sep 2019) and he bet me a round 
    of beers that I couldn't get more than +5 points on CodeReview for posting
    the solution to this question.

    DESCRIPTION:
    (Inspired by Chandler's GameOfCups with Joey in "Friends".)

    Program gets 5-digit zipcode from user. (Assume user won't make a mistake, and
    will enter exactly 5 digits). Program awards points based on a series of 
    rules, and reports the total points earned at the end. The 8 rules are 
    embedded as comments in the code. For each rule, besides adding points (or 
    not) to the 
   total, rule displays "Rule _ got _ points, so total is now _" (It prints this 
   even if rule added 0 points to total).

    RULES
       +5  when first and last digit match
       +6  when second digit is twice the first AND third digit is greater than 
           second or fourth digit
       +7  if any 7 is in the zipcode
       +8  when there's no "13" in MIDDLE the zipcode
       +9  when all three middle digits match
       +10 when third and fourth digits match
       +11 when zipcode is palindrome (12121 == 12121, while 12345 != 54321)
""" 

Why are you not using functions?

Yes, it's true that the individual steps here are small and simple. That's no reason not to put them into functions that express what they do. When you add that little bit of abstraction, it makes the program easier to understand. When you add a docstring to your function it makes things even easier, and gives you a place to help FutureYou understand what you were doing or not doing.

def get_zipcode() -> str:
    """ Get a zip code from the user.

        A zip code is \d{5}. No validation, though, because of the spec.
    """
    zipcode = input("Enter your zipcode: ")
    return zipcode

def rule1(zipcode: str) -> int:
    """ +5 when first and last digit match """
    points = 5 if zipcode[0] == zipcode[-1] else 0
    return points

def rule2(zipcode: str) -> int: ...
def rule3(zipcode: str) -> int: ...
def rule4(zipcode: str) -> int: ...
def rule5(zipcode: str) -> int: ...
def rule6(zipcode: str) -> int: ...
def rule7(zipcode: str) -> int: ...

Why are you not following Python's convention for main?

Simple rule: the only time you don't do this is inside python -c "<code>".

if __name__ == '__main__':
    total_points = 0
    zipcode = get_zipcode()

    points = rule1(zipcode)
    total_points += points
    print(f"Rule 1 got {points} points, so total is now {total_points}")

    ...

    points = rule7(zipcode)
    total_points += points
    print(f"Rule 7 got {points} points, so total is now {total_points}")

    print(f"{zipcode} got {total_points} points!")

Why are you not making your code testable?

Especially when you are asking for suggestions on CodeReview, it's important to have confidence that some suggested changes doesn't cause a failure. So encapsulate your code in functions that you can call in some kind of test harness.

In this case, it means that your points calculation should also be a function:

if __name__ == '__main__':
    zipcode = get_zipcode()
    total_points = game_of_cups(zipcode)
    print(f"{zipcode} got {total_points} points!")

Once you have that, you can write some test cases:

def game_of_cups(zipcode: str) -> int:
    """ Compute total points according to the rules. Print the score
        of each rule, with a running total.

       +5  when first and last digit match
       +6  when second digit is twice the first AND third digit is greater than 
       second or fourth digit
       +7  if any 7 is in the zipcode
       +8  when there's no "13" in MIDDLE the zipcode
       +9  when all three middle digits match
       +10 when third and fourth digits match
       +11 when zipcode is palindrome (12121 == 12121, while 12345 != 54321)

    >>> game_of_cups('12345') == 8
    Rule 1 got 0 points, so total is now 0
    Rule 2 got 0 points, so total is now 0
    Rule 3 got 0 points, so total is now 0
    Rule 4 got 8 points, so total is now 8
    Rule 5 got 0 points, so total is now 8
    Rule 6 got 0 points, so total is now 8
    Rule 7 got 0 points, so total is now 8
    True
    >>> game_of_cups('12321')
    Rule 1 got 5 points, so total is now 5
    Rule 2 got 6 points, so total is now 11
    Rule 3 got 0 points, so total is now 11
    Rule 4 got 8 points, so total is now 19
    Rule 5 got 0 points, so total is now 19
    Rule 6 got 0 points, so total is now 19
    Rule 7 got 11 points, so total is now 30
    30
    """
    total_points = 0
    ... etc ...

Then you can run python -m doctest game-of-cups.py and see some discouraging news:

(so) aghast@laptop:~/Code/so$ python -m doctest game-of-cups.py 
**********************************************************************
File "/home/aghast/Code/so/game-of-cups.py", line 92, in game-of-cups.game_of_cups
Failed example:
    game_of_cups('12345')
Expected:
    Rule 1 got 0 points, so total is now 0
    Rule 2 got 0 points, so total is now 6
    Rule 3 got 0 points, so total is now 6
    Rule 4 got 8 points, so total is now 14
    Rule 5 got 0 points, so total is now 14
    Rule 6 got 0 points, so total is now 14
    Rule 7 got 0 points, so total is now 14
    14
Got:
    Rule 1 got 0 points, so total is now 0
    Rule 2 got 6 points, so total is now 0
    Rule 3 got 0 points, so total is now 0
    Rule 4 got 8 points, so total is now 8
    Rule 5 got 0 points, so total is now 8
    Rule 6 got 0 points, so total is now 8
    Rule 7 got 0 points, so total is now 8
    8
**********************************************************************
File "/home/aghast/Code/so/game-of-cups.py", line 101, in game-of-cups.game_of_cups
Failed example:
    game_of_cups('12321')
Expected:
    Rule 1 got 5 points, so total is now 5
    Rule 2 got 6 points, so total is now 11
    Rule 3 got 0 points, so total is now 11
    Rule 4 got 8 points, so total is now 19
    Rule 5 got 0 points, so total is now 19
    Rule 6 got 0 points, so total is now 19
    Rule 7 got 11 points, so total is now 30
    30
Got:
    Rule 1 got 5 points, so total is now 5
    Rule 2 got 6 points, so total is now 11
    Rule 3 got 0 points, so total is now 11
    Rule 4 got 8 points, so total is now 19
    Rule 5 got 0 points, so total is now 19
    Rule 6 got 0 points, so total is now 19
    Rule 7 got 0 points, so total is now 19
    19

Which leads me to point out that:

If you have functions, you can insert doctests in lots of places.

Like this:

def rule2(zipcode: str) -> int:
    """ +6  when second digit is twice the first AND third digit is greater 
            than second or fourth digit

    >>> rule2('12345')
    6
    >>> rule2('13431')
    0
    >>> rule2('12321')
    6
    """
    points = 6 if (int(zipcode[1]) * 2) > int(zipcode[0]) and (int(zipcode[2]) > int(zipcode[1]) or int(zipcode[2]) > int(zipcode[3])) else 0
    return points

And yeah, your code here is wrong. You compute d1 * 2 > d0 but the rule says "is twice", which means ==. Also, the second clause could be interpreted two ways. You have interpreted it as d1 < d2 or d3 < d2, but it's probably worth some emphasis or clarification or something.

Also here:

def rule7(zipcode: str) -> int:
    """ +11 when zipcode is palindrome (12121 == 12121, while 12345 != 54321)
    >>> rule7('12321')
    11
    """
    # points = 11 if zipcode == reversed(zipcode) else 0
    points = 11 if zipcode == zipcode[::-1] else 0
    return points

This fails because reversed() returns a reversed iterator object, not a string. Using the [::-1] reversing idiom works, though.

Finally, some repetition!

Finally, you'll notice that while the structure of the rules is similar, the contents of the rule functions is all different. I'm going to ignore that, since this is a pretty simple script.

However, the high-level gap_of_cups function has got 7 calls to rule-functions which look identical except for the rule number in two places.

    points = rule1(zipcode)
    total_points += points
    print(f"Rule 1 got {points} points, so total is now {total_points}")

We can take advantage of the fact that in Python, functions are first-class objects. Just put the functions in an iterable, and iterate over them.

However, there's the question of how to print "Rule 1". We could use enumerate to keep an integer value, or we could actually use the f.__name__ attribute of function objects. Since it's shorter and easier, let's just use enumerate:

for i, rule in enumerate([rule1, rule2, rule3, rule4, rule5, rule6, rule7]):
    points = rule(zipcode)
    total_points += points
    print(f"Rule {i} got {points} points, so total is now {total_points}")
\$\endgroup\$
2
\$\begingroup\$

Like other answers, this one uses a separate function for each rule. Unlike the others it automatically collects the rules, so you don't need to keep a list or dictionary of all the rules. Each function implementing a rule has a name that matches a pattern -- it ends with "_rule". calculate_score() the scans globals() looking for the names of functions that match the pattern. The same kind of thing is done by some testing frameworks.

def first_and_last_digits_match_rule(zipcode):
    '''+5 when first and last digit match'''
    return 5 if zipcode[0] == zipcode[-1] else 0

def second_rule(zipcode):
    '''
    +6 when second digit is twice the first AND 
    third digit is greater than second or fourth digit
    '''
    return 6 if zipcode[:2] in ('12','24','36','48') and not (zipcode[1] <= zipcode[2] >= zipcode[3]) else 0

def any_7_rule(zipcode):
    '''+7 if any 7 is in the zipcode'''
    return 7 if '7' in zipcode else 0

def no_13_rule(zipcode):
    '''+8 when there's no "13" in MIDDLE the zipcode'''
    return 8 if '13' not in zipcode[1:-1] else 0

def triplet_rule(zipcode):
    '''+9 when all three middle digits match'''
    return 9 if zipcode[1] == zipcode[2] == zipcode[3] else 0

def digits_3_and_4_match_rule(zipcode):
    '''+10 when third and fourth digits match'''
    return 10 if zipcode[2] == zipcode[3] else 0

def palindrome_rule(zipcode):
    '''+11 when zipcode is palindrome (12121 == 12121, while 12345 != 54321)'''
    return 11 if zipcode == zipcode[::-1] else 0


def calculate_score(zipcode):
    score= 0

    rules = [(name,func) for name,func in globals().items() if name.endswith("_rule") and callable(func)]

    for name,rule in rules:
        points = rule(zipcode)
        score += points
        print(f"""The "{name[:-5].replace('_',' ')}" rule got {points} points. Score is now {score}.""")

    return score

if __name__ == "__main__":
    zipcode = input("Enter zipcode")
    calculate_score(zipcode)
\$\endgroup\$
  • \$\begingroup\$ Nice, but how do you ensure rules and their results are printed in ascending rule number order? \$\endgroup\$ – AJNeufeld Sep 19 at 13:38
  • \$\begingroup\$ @AJNeufeld I don't see a requirement to run/print them in order. However, the line rules = [ ... ] can be changed to rules = sorted( ... ) to sort the rules. Change the rule function names to rule1, rule2, etc. Or provide a key function for more complex sorting. \$\endgroup\$ – RootTwo Sep 19 at 17:18
  • \$\begingroup\$ It doesn’t explicitly say they have to be sorted, but the OP’s code did have them in rule # order, so it seems implicit. It does say to output Rule _ got ..., not The "_" rule got ... so perhaps you should have f"Rule {name[-1:]} got ..." \$\endgroup\$ – AJNeufeld Sep 19 at 18:42
  • \$\begingroup\$ Actually, since you're using f-strings, you're Python 3.6+ ... and since dictionaries in Python 3.6+ are insertion ordered, the globals() actually maintains source-code ordering of the rules (at least in my Python 3.7 tests). No need for rules = sorted( ... , key=...). \$\endgroup\$ – AJNeufeld Sep 19 at 18:55
  • \$\begingroup\$ I thought globals() might return them in order, but didn't have time to check. \$\endgroup\$ – RootTwo Sep 19 at 19:01

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.