5
\$\begingroup\$

I wrote simple code that let users log in, see what items are available in "shop", and make a purchase if he/she has enough coins.

Because the code uses csv file, if you want to test the code, you will need to create one. Run following

import csv

#Path to the folder + file name
file_path = input()

#Each sublist contains following elements:
#[username, password, coins, item1,item2,item3,......]
some_users = [
    ['User1','VeryStrongPassword123','999' ,*['Sword','Shield']],
    ['Bob'  ,'123456'               ,'1223',*[]],
    ['asdf1','12jknsdf'             ,'0'   ,*[]]]

#It must be an csv file.
with open(file_path,'w') as file:
    csv_writer = csv.writer(file)
    for row in some_users:
        csv_writer.writerow(row)

Code itself

import os
import csv
import pandas as pd

#Note that it must be the csv file you've just created
file_path = input()

ITEMS_DATA ={'Item': 'Price',
             'Sword': 100,
             'Shield': 510,
             'Horse': 1200,
             'Big e': 99999999}


class CSV():

    def __init__(self,path):
        self.path = path


    def read(self):
        with open(self.path) as file:
            data = list(csv.reader(file))
        return data


    def write(self,new_data):
        with open(self.path,'w') as file:
            csv_writer = csv.writer(file)
            for row in new_data:
                csv_writer.writerow(row)


class Shop:


    def __init__(self, ID):
        self.ID = ID
        self.username, \
        self.password, \
        self.coins, \
        self.my_items = retrieve_user_data(ID)

        self.coins = int(self.coins)


    def update_data(self):
        csv_file = CSV(file_path)
        all_data = csv_file.read()
        user_data = all_data[self.ID]
        user_data_updated = [user_data[0],user_data[1],self.coins] + self.my_items
        all_data[self.ID] = user_data_updated
        csv_file.write(all_data)
        return True


    def show_all_items(self):
        body = ''
        for item in ITEMS_DATA:
            body += f'{item:10} {ITEMS_DATA[item]} \n'
        print(body)


    def purchase(self,item):
        if (item not in ITEMS_DATA or item == "Item"):
            return "Item not found"
        else:
            item_cost = ITEMS_DATA[item]
            if self.coins < item_cost:
                return "Not enough coins"
            else:
                verify_purchase = input(f"You will spend {item_cost} coins. Want to proceed? Y/N \n")
                if verify_purchase.lower() == 'y':
                    self.coins -= item_cost
                    self.my_items.append(item)
                    self.update_data()


def create_user(username,password,coins,items):
    csv_file = CSV(file_path)
    data = csv_file.read()
    data.append([username,password,coins,*items])
    csv_file.write(data)
    return True


def retrieve_user_data(ID):
    users_csv = CSV(file_path)
    row = users_csv.read()[ID]
    username,password,coins = row[:3]
    items = row[3:]
    return (username, password, coins, items)


def login():
    users_csv = CSV(file_path)
    users_data = users_csv.read()
    all_usernames = [x[0] for x in users_data]
    #
    login = input("Username: ")
    password = input("Password: ")
    attempt = 0
    while True:
        if login in all_usernames:
            user_ID = all_usernames.index(login)
            if password == users_data[user_ID][1]:
                return (login,password,user_ID)
            else:
                attempt += 1
                if attempt == 3:
                    return False
                print("Incorrect username or password. Try one more time: ")
                login = input("Username: ")
                password = input("Password: ")
        else:
            attempt += 1
            if attempt == 3:
                return False
            print("Incorrect username or password. Try one more time: ")
            login = input("Username: ")
            password = input("Password: ")


if __name__ == '__main__':
    login_attempt = login()
    if login_attempt != False:
        user_ID = login_attempt[-1]
        instance_variable = Shop(user_ID) 
    else:
        print("Log-in failed.")

What can be improved?

Written in Python 3.6.5

\$\endgroup\$
  • \$\begingroup\$ Is this an exercise in CSV handling or an exercise in shop building? We can work with either, but the answers could be hugely different based on your goal. \$\endgroup\$ – Mast Oct 28 '19 at 5:01
  • \$\begingroup\$ @Mast it's the latter. So if you have any suggestions about how to handle data without using CSV, I would be glad to hear them. I used CSV because it's the first thing that came to my mind, but I suppose there are better options available. \$\endgroup\$ – Nelver Oct 28 '19 at 5:16
5
\$\begingroup\$

I'll be keeping a bit general with this. Over the whole, looks good. That means I won't be doing a line-by-line, but instead lifting out the points grabbing my attention.

if __name__ == "__main__":

You should guard your code. It's a good thing that you already have one, but not all your code is inside it. For example, there's a rogue input() statement above ITEMS_DATA. Just do that inside the guard as well, and feed the result to any functions that need it.

Naming

You have a Shop class. What data does this contain? It's attributes set in __init__() are: ID, username, password, coins, my_items. These aren't things about the shop, these are actually properties of the player buying the items.

The actual shop's items are all in ITEM_DATA, which is the correctly named global.

So if you see me refer to a Player.purchase() method later, you'll know what I'm referring to ;)

print()ing in a class method

is generally a bad idea. Instead, return a string which can then be printed. Examples of methods that should really be doing this are show_all_items(), which really should be a function get_all_items() which returns a list for consumption, or render_all_items() which returns a string you can then print from outside.

Same issue but different in purchase(), which calls input(). A class' methods shouldn't be about control flow, they should only handle the class' data and it's primary purpose. A pseudocode example could be:

def purchase(self, item):
    if not item_exists() or not can_purchase():
        return False
    self.add_item(item)
    self.coins -= item_cost
# Calling code:
item = input("What do you want to buy?")
if person.purchase(item):
    print(f"Bought a {item}!")
else:
    print(f"Oh noes! Failed to buy {item}!")

If you want to have some excersize with Exceptions instead, you could also raise a InsufficientMoneyError or ItemDoesNotError exception, and catch those in your calling code, using them to print the correct problem. But I'm at risk of veering off-topic here, and if you want a howto, ask at StackOverflow. Link it in a comment and I'll happily type it out.

Login security

I'm going to assume you're not planning on using this login system for "real" purposes. A long diatribe on login security would also be offtopic, I think. Just be aware that this sort of system should not go beyond private toys like this, and even then it might be worth it to factor it out a bit clearer so you can replace it with a real system if the need ever arises.

Generator Expressions

You've already used a list comprehension for all_usernames, but you aren't using this tool to it's full potential yet. show_all_items() comes to mind. If we were to make this a function that returns a single string, it could look like this:

@staticmethod
def all_items() -> str:
    return "\n".join(f"{key: <10}:{value}" for key, value in ITEMS_DATA.items())

The formatting string "{key: <10}" right-fills the key to length 10 with spaces. If you want both keys and values of a dict, you should use dict.items() to get both instead of indexing into it - it's faster, but more important it's more readable. Then we set up a generator expression - the big brother of the list comprehension - and feed that to a str.join() which glues them together with a newlines between every line. Then it returns the one big string, so you can just call print(shop.all_items()) if you want to.

Also, note the @staticmethod decorator. You're not using any of the class' variables. So we don't need access. You could of course split it off, but if you feel the functionality is tied to the class in a close way, @staticmethod is the way to go.

Annotations

This is never required for Python, but it's a good habit to get into. Most importantly, it makes you think about what your functions do, and what you functions should return. Smart IDE's can also warn you about feeding in the wrong values to a function, or expecting the wrong return values.

A minor example is in the all_items() method I showed upwards a bit - it shows that you should expect a string as a return value. Another would be:

from typing import Union
# ...Other Code...
def purchase(self, item:str) -> Union[str, None]:

Which shows that this function either returns a string, or None. And this begs the question - why are we returning different things? Perhaps we should refactor this function to either return strings only, or None only?

And the answer is of course, yes we should. I'll leave that decision to you - there's no "better practice" which forces you to use either. My personal choice would be what I did a bit upwards, and return a bool indicating success or failure of the purchase.

(Note for other reviewers: Yes, I could have used Optional here. But semantically, my not very humble opinion is that that should be restricted to arguments. Sue me. )

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks for the review! Unfortunately, I didn't get one thing: what does @staticmethod do? Seems like even if I don't use it, I still call functions from the class without instantiating it. And one more question: Would you advise to use annotations for every function? \$\endgroup\$ – Nelver Oct 28 '19 at 11:33
  • 1
    \$\begingroup\$ @trentcl None is a special case for type annotations, and it has the same effect as NoneType when used. For any X, Optional[X] is syntactically identical to Union[X, None]. So yes, Optional technically has nothing to do with default arguments, but all arguments with default values could literally be considered "optional". Also see PEP484 and PEP526 for more about the how and why. Note that not using Optional for return values is my personal choice only, and not a broader practice as far as I know. \$\endgroup\$ – Gloweye Oct 28 '19 at 13:04
  • 2
    \$\begingroup\$ @Nelver My habit has been: all function parameters/return types should be fully annotated, as should anything else whose type may be ambiguous given the context. If I look at code and have a moment where I can't tell what types are involved, I'll add an annotation for reference. I wouldn't annotate literally every variable though. At some point the annotations become noise. \$\endgroup\$ – Carcigenicate Oct 29 '19 at 3:18
  • 1
    \$\begingroup\$ @Carcigenicate This is what I've been thinking about too. Annotations must be helpful in a lot of cases, but seems like it doesn't make sense to use them when it is obvious from the context what the type of variable must be. \$\endgroup\$ – Nelver Oct 29 '19 at 4:21
  • 2
    \$\begingroup\$ @Nelver Ya. The type system is only helpful until it becomes bulky and in your way. Just appreciate the fact that Python doesn't force static typing, but also don't disregard the benefits of having known types at certain times. \$\endgroup\$ – Carcigenicate Oct 29 '19 at 4:25
3
\$\begingroup\$

Your Shop constructor kind of threw me off guard when I read it:

def __init__(self, ID):
    self.ID = ID
    self.username, \
    self.password, \
    self.coins, \
    self.my_items = retrieve_user_data(ID)

I find this to be very confusing to read. It isn't obvious what's going on until you get to the bottom and realize that it's a tuple deconstruction. I'd probably use an intermediate object to make it read cleaner. I'd also add type annotations:

def __init__(self, ID: str):
    self.ID: str = ID  # Really, this should be lowercase, but that shadows the built-in unfortunately.

    username, password, raw_coins, items = retrieve_user_data(ID)

    self.username: str = username
    self.password: str = password
    self.coins: int = int(raw_coins)
    self.my_items: str = items

It makes it a little longer, but I think it reads much better.


I think purchase can be improved quite a bit as well. The main thing that I see wrong with it is how you're handling errors. Returning a string message is messy. If the caller wants to know what went wrong (and handle it programmatically), they need to match the string against known message which is both slower, and more error prone than other methods.

To fix these issues, I'd decide if I needed to handle multiple different errors, or just one error. If I only had one error, I'd change your function to return a bool to indicate success. You have two possible errors though that may need to be handled differently. I think the cleanest solution would be to have an enum of PurchaseTransactionOutcomes that lets the caller know what happened:

from enum import Enum

class PurchaseTransactionOutcome(Enum):
    ITEM_NOT_FOUND = 0
    INSUFFICIENT_COINS = 1
    CANCELLED_BY_USER = 2
    SUCCESS = 3

The benefits of this are it'll be faster (although that's not likely an issue in most cases), and it's more self documenting. If the user sees that the method returns a PurchaseTransactionOutcome, they can easily check to see what all of the possible outcomes are that they'll need to handle. It also prevents typos. If you typed the string wrong in either the method or the calling code, any checks against a certain outcome would fail. Enums can't lead to silent typos though. PurchaseTransactionOutcome.INSUFICIENT_COINS would cause a error before the code even runs.

And to handle the nesting, I'd just switch to elifs. After making the above changes, I'd write this method closer to:

def purchase(self, item) -> PurchaseTransactionOutcome:
    if (item not in ITEMS_DATA or item == "Item"):
        return PurchaseTransactionOutcome.ITEM_NOT_FOUND

    else:
        item_cost = ITEMS_DATA[item]
        if self.coins < item_cost:
            return PurchaseTransactionOutcome.INSUFFICIENT_COINS

        else:
            verify_purchase = input(f"You will spend {item_cost} coins. Want to proceed? Y/N \n")
            if verify_purchase.lower() == 'y':
                self.coins -= item_cost
                self.my_items.append(item)
                self.update_data()
                return PurchaseTransactionOutcome.SUCCESS

            else:
                return PurchaseTransactionOutcome.CANCELLED_BY_USER

Yes, for your case here, this is gross overkill. It is something to keep in mind though once you start writing more "real" code.

Now to check for success, you can write:

result = user.purchase("sword")

if result != PurchaseTransactionOutcome.SUCCESS:
    . . .  # Figure out what error and handle it

In ITEMS_DATA, for some reason you're storing header names inside the "database". This is forcing you to do weird things like ... or item == "Item" checks. I'm guessing you're doing this to help show_all_items. I would take the header names out and just have show_all_items handle that:

ITEMS_DATA = {'Sword': 100,
              'Shield': 510,
              'Horse': 1200,
              'Big e': 99999999}

def show_all_items(self):
    body = f'{"Name":10} Price\n'
    for item_name, price in ITEMS_DATA.items():
        body += f'{item_name:10} {price} \n'
    print(body)

Or to avoid duplication, you can add the headers right before the data is iterated over:

def show_all_items(self):
    body = ''
    for item_name, price in [["Name", "Price"]] + list(ITEMS_DATA.items()):
        body += f'{item_name:10} {price} \n'
    print(body)

I agree with @Gloweye though, all these methods shouldn't be printing. Ideally, all the data a function needs should be taken in as parameters, and all the data that a function produces should be simply returned. Eventually you need to call input and print somewhere, but be choosy where that somewhere is. Calling functions like print and input in arbitrary spots will make it more difficult to adapt to changed later because you've baked into your code exactly how the program is able to take and produce data. If you switch to a full UI later, you're going to have to go around and change every function that calls print and input.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks for you review! May I ask you why you used Enum when creating PurchaseTransactionOutcome? I just tried to create the same class without using Enum, and I don't observe any difference. \$\endgroup\$ – Nelver Oct 29 '19 at 4:18
  • \$\begingroup\$ And one more: suppose that instead of class we would use something like this PurchaseTransactionOutcome = {'ITEM_NOT_FOUND': 0, 'INSUFFICIENT_COINS': 1, 'CANCELLED_BY_USER': 2, 'SUCCESS': 3}. Why would that be a bad idea? \$\endgroup\$ – Nelver Oct 29 '19 at 4:30
  • \$\begingroup\$ @Nelver An enum represents a closed set of possible states. It is entirely possible to mostly emulate an enum using loose variables and still get many of the benefits. Inheriting from enum is just a shortcut that gives you some free functionality. You can more easily test if a member is a part of an enum, and you get some formatting help when stringifying your enum. It's not necessary, but it can be helpful. If you read over the docs for Enum, you can see all that is adds. \$\endgroup\$ – Carcigenicate Oct 29 '19 at 4:33
  • \$\begingroup\$ @Nelver And it wouldn't be a bad idea; that's actually how I emulate enumerations when I'm writing Clojure. It works fine. \$\endgroup\$ – Carcigenicate Oct 29 '19 at 4:34
  • 1
    \$\begingroup\$ @Nelver Sorry, I should also add, using a dictionary is fine, but it doesn't give you advance warning of typos. If you type an enum wrong, you'll get warnings from your IDE before your code is even run. If you use a dictionary and type a key wrong though, you'll get a key error at runtime. \$\endgroup\$ – Carcigenicate Oct 29 '19 at 4:42

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.