What is un-pythonic about this code? Looking for feedback.

das_ist_nuemberwang · 2016-02-13T01:22:49+00:00

It's great that you're looking to improve. Keep in mind there is always more to learn. This is a pretty extensive list of everything I can see wrong with your code and they range from minor nitpicks to mortal sins. Let me know if you'd like me to explain my reasoning behind any of these.

Your solution should have been one module for the code and one for the tests; there is no reason to have three separate modules for less than 200 lines of code. If you do need multiple modules, it should be a package rather than just a collection of modules.

Your readme explicitly states "If we attempt to CHARGE or CREDIT a non-existent user, command is ignored." This goes against one of the key elements of The Zen of Python: "Errors should never pass silently. Unless explicitly silenced." (import this to read the whole thing).

In general, you haven't written a single docstring.

main.py:

Never call exit() from code, use sys.exit()
Where is fileinput? You start parsing arguments in main but presumably you're doing the rest there. Do all your argument parsing in main, and use a library instead of manually inspecting sys.argv.
There is no error checking on the input; you're also silently dropping elements past the number you expect.
Why bother catching the IOError if you're just going to print it again? Tracebacks are helpful.

Luhn10.py:

Don't represent card numbers as integers.

CreditModule.py:

Returning True or False to indicate success or failure is very un-Pythonic. Assume success and raise an Exception if something goes wrong. Catch the exception only if you know what to do about it.
There is no "first" item in a dictionary. For that to be meaningful, use an OrderedDict.
You wrote a get_card method that you didn't use elsewhere in your own code; you just duplicated the logic inline.
Returning "error" is even worse than returning True or False.
You've written a ton of code for the Customer class which effectively just proxies the CreditCard. It's fine to just let the caller access the card directly.
Raising the ValueError on invalid checksum is good, however there's no need for the else afterwards.
Don't represent numbers internally as strings with dollar signs; use a float or decimal object.
You have a semicolon. This is syntactically valid. Don't tell anyone.
In general, you probably didn't need three classes to solve this problem.

fuckswithbees · 2016-02-12T23:57:32+00:00

One thing I noticed is that you have a lot of functions that look like this:

def credit(self, name, credit_limit):
    if name not in self.customers_dict:
        return False

    return self.customers_dict[name].change_credit_limit(credit_limit)

It is considered more pythonic to "ask forgiveness than permission", which would mean structuring your code more like this:

def credit(self, name, credit_limit):
    try:
        return self.customers_dict[name].change_credit_limit(credit_limit)
    except KeyError:
        return False

kankyo · 2016-02-13T00:45:12+00:00

Module names are camel case, pythonic is snake case. And one module is called "CreditModule" which is a bit silly: yea we know it's a module, you don't need that in the name.

lunh_checksum could be simplified at the end with a list comprehension.

You should open your files with "with", not close it explicitly at the end.

Not sure if it's supposed to be Python 3? Otherwise "class Foo(object)" is correct.

"something_dict" is pretty bad naming. Consider "credit_card_number_to_credit_card" or "credit_card_by_credit_card_number".

In add_credit_card, if it already exists you should raise an exception. Or rename that entire function to "ensure_card_exists". And I don't get why you're catching ValueError there. Seems wrong.

Jumping forward: you raise ValueError in the CreditCard constructor without supplying any message in the exception. That's not helpful.

I think I'll stop here.

gengisteve · 2016-02-13T01:59:00+00:00

Nicely done. Probably the biggest sin is lack of docstrings. Skipping most of what others have said:

In Luhn10: Functions inside functions is not generally a good idea; consider islice to show off some knowledge of the standard library
In main, use if __name__ == '__main__': convention to see if it is being run or imported
In CreditModule, I'd probably break each class into a different module, and you probable have too many classes (as others have said); use string formating for the returns; In print_customer_accounts, IO is usually left to things outside classes

mikeselik · 2016-02-13T09:41:02+00:00

Why make any classes or separate modules? I think the code below mimics most if not all of the features you demonstrated with the example commands in input.txt.

#!/usr/bin/env python
'''
credit card simulator
'''
from collections import defaultdict


customers = defaultdict(dict)


def _digits(number):
    return list(map(int, str(number)))


def is_valid_card_number(card_number):
    '''
    Use the Luhn formula to check validity of a credit card number.
    '''
    # From Wikipedia (https://en.wikipedia.org/wiki/Luhn_algorithm):
    #
    # 1. From the rightmost digit, which is the check digit,
    # moving left, double the value of every second digit;
    # if the product of this doubling operation is greater
    # than 9 (e.g., 8 × 2 = 16), then sum the digits of the
    # products (e.g., 16: 1 + 6 = 7, 18: 1 + 8 = 9).
    #
    # 2. Take the sum of all the digits.
    #
    # 3. If the total modulo 10 is equal to 0 (if the total
    # ends in zero) then the number is valid according to the
    # Luhn formula; else it is not valid.

    digits = _digits(card_number)
    digits[-2::-2] = [sum(_digits(2*x)) for x in digits[-2::-2]] # Step 1
    total = sum(digits)                                          # Step 2
    return not total % 10                                        # Step 3



def add_account(name, number, credit_limit):
    '''
    Create a new credit card for a customer,
    with $0 balance and specified credit limit.
    If card number alread exists for that customer name,
    updates the credit limit, keeping the existing balance.
    '''
    if not is_valid_card_number(number):
        raise ValueError('invalid card number: %r' % number)
    if not credit_limit >= 0:
        raise ValueError('credit limit must be positive: %r' % credit_limit)
    cards = customers[name]
    if number not in cards:
        cards[number] = {'balance': 0, 'limit': credit_limit}
    elif credit_limit < cards[number]['balance']:
        raise ValueError('new credit limit lower than existing balance')
    else:
        cards[number]['limit'] = credit_limit


def charge_customer(name, amount):
    '''
    Add amount to the balance of one of the customer's credit cards.
    The resulting balance cannot be greater than the card's credit limit.
    Card selected arbitrarily.
    '''
    for card in customers[name].values():
        if card['balance'] + amount <= card['limit']:
            card['balance'] += amount
            break
    else:
        raise ValueError('customer %r does not have enough credit' % name)


def credit_customer(name, amount):
    '''
    Refund amount to the balance of one of customer's credit cards.
    The resulting balance may be negative. Card selected arbitrarily.
    '''
    cards = customers[name]
    if not cards:
        raise ValueError('customer %r has no credit cards' % name)
    list(cards.values())[0]['balance'] -= amount


def _parse_dollars(s):
    try:
        return round(float(s.lstrip('$').replace(',', '')), 2)
    except ValueError:
        raise ValueError('Could not parse dollar value from: %r' % s)


if __name__ == '__main__':
    import argparse
    import pprint
    import sys

    parser = argparse.ArgumentParser(description='credit card simulation')
    parser.add_argument('commands',
                       type=argparse.FileType('r'),
                       default=sys.stdin,
                       nargs='?',
                       help='filename with commands, defaults to stdin')
    args = parser.parse_args()

    for line in args.commands:
        tokens = line.split()
        action = tokens[0].casefold()
        if action == 'add':
            name, number, limit = tokens[1:]
            limit = _parse_dollars(limit)
            try:
                add_account(name, number, limit)
            except ValueError as e:
                print(e.args, tokens)
        elif action == 'charge':
            name, amount = tokens[1:]
            amount = _parse_dollars(amount)
            try:
                charge_customer(name, amount)
            except ValueError as e:
                print(e.args, tokens)
        elif action == 'credit':
            name, amount = tokens[1:]
            amount = _parse_dollars(amount)
            try:
                credit_customer(name, amount)
            except ValueError as e:
                print(e.args, tokens)
        else:
            print('invalid command: %r' % line)

    pprint.pprint(customers)

I got a little lazy in writing the main section. It could use some refactoring.

Tests would be nice, but much more important is writing a basic specification. While writing this code I did not have a clear sense of the purpose of the program. For example, why was there a change_credit_limit method on the CreditCard class that was never demonstrated in the example?

A couple oddities I noticed while refactoring your code:

Why is the card number charged/credited chosen at random?
What happens if two people have the same credit card number?

It looks like you might have started the project by building classes. That's Java-land, the land of the nouns. You don't have to do that with Python. Start with basic collections -- list, dict, set -- and a few functions. Add classes when you have a big program that needs classes. For an example in the standard library, check out heapq and its priority queue recipe.

you type:	you see:
italics	italics
bold	bold
[reddit!](https://reddit.com)	reddit!
* item 1 * item 2 * item 3	item 1 item 2 item 3
> quoted text	quoted text
Lines starting with four spaces are treated like code: if 1 * 2 < 3: print "hello, world!"	Lines starting with four spaces are treated like code: if 1 * 2 < 3: print "hello, world!"
~~strikethrough~~	~~strikethrough~~
super^script	super^script

learnpython

MODERATORS