12
\$\begingroup\$

I built a shop system for a Python text RPG I'm making. It repeats itself more than I would like. I was thinking I could use dictionaries to make it better but I'm not sure how. Anyways, how is it?
There are 5 shop levels, each having different things to buy. So based on the shop level I set what you can buy, as well as how much it is.

def shop(shopLvl, playerGp, playerInventory):
    print("What would you like to buy?\n")
    if shopLvl == 1:
        shopWeapon = "1. Longsword"
        weaponCost = 20
        shopArmor = "2. Leather Armor"
        armorCost = 30
        shopPotion = "3. Potion"
        potionCost = 5
        shopElixer =  "4. Elixer"
        elixerCost = 5
    if shopLvl == 2:
        shopWeapon = "1. Greatsword"
        weaponCost = 30
        shopArmor = "2. Chainmail Armor"
        armorCost = 45
        shopPotion = "3. Potion"
        potionCost = 5
        shopElixer =  "4. Elixer"
        elixerCost = 5
    if shopLvl == 3:
        shopWeapon = "1. Crested Blade"
        weaponCost = 45
        shopArmor = "2. Plate Armor"
        armorCost = 60
        shopPotion = "3. Super Potion"
        potionCost = 10
        shopElixer =  "4. Super Elixer"
        elixerCost = 10
    if shopLvl == 4:
        shopWeapon = "1. Diamond Sword"
        weaponCost = 75
        shopArmor = "2. Diamond Armor"
        armorCost = 125
        shopPotion = "3. Super Potion"
        potionCost = 10
        shopElixer =  "4. Super Elixer"
        elixerCost = 10
    if shopLvl == 5:
        shopWeapon = "1. Enchanted Sword"
        weaponCost = 150
        shopArmor = "2. Angelic Armor"
        armorCost = 225
        shopPotion = "3. Mega Potion"
        potionCost = 20
        shopElixer =  "4. Mega Elixer"
        elixerCost = 20
    
    shopItems = {
        shopWeapon : weaponCost,
        shopArmor : armorCost,
        shopPotion : potionCost,
        shopElixer : elixerCost
    }
        
    print(shopItems,"\nYour gp:", playerGp)
    shopChoice = input("1/2/3/4")
    
    if shopChoice == '1':
        if weaponCost < playerGp:
            print("You bought a", shopWeapon, "for", weaponCost, "gp!\n")
            time.sleep(0.8)
            playerGp = playerGp - weaponCost
            print("You now have", playerGp, "gp left\n")
            time.sleep(0.8)
            playerWeapon = shopWeapon
            
        elif weaponCost > playerGp:
            print("Not enough gold\n")
        else:  print("Error\n")
    if shopChoice == '2':
        if armorCost < playerGp:
            print("You bought a", shopArmor, "for", armorCost, "gp!\n")
            time.sleep(0.8)
            playerGp = playerGp - armorCost
            print("You now have", playerGp, "gp left\n")
            time.sleep(0.8)
            playerArmor = shopArmor
        elif armorCost > playerGp:
            print("Not enough gold\n")
        else: print('Error\n')
        
    if shopChoice == '3' :
        if potionCost < playerGp:
            print("You bought a", shopPotion, "for", potionCost, "gp!\n")
            time.sleep(0.8)
            playerGp = playerGp - potionCost
            print("You now have", playerGp, "gp left\n")
            time.sleep(0.8)
            if shopPotion == "3. Potion":
                playerInventory["1.Potion"] = playerInventory["1.Potion"] + 1
            if shopPotion == "3. Super Potion":
                playerInventory["3.Super Potion"] = playerInventory["3.Super potion"] + 1
            if shopPotion == "3. Mega Potion":
                playerInventory["3.Mega Potion"] = playerInventory["3.Mega Potion"] + 1
        elif potionCost < playerGp:
            print("Not enough gold\n")
    if shopChoice == '4' :
        if elixerCost < playerGp:
            print("You bought a", shopElixer, "for", elixerCost, "gp!\n")
            time.sleep(0.8)
            playerGp = playerGp - elixerCost
            print("You now have", playerGp, "gp left\n")
            time.sleep(0.8)
            if shopElixer == "4. Elixer":
                playerInventory["2.Elixer"] = playerInventory["2.Elixer"] + 1
            if shopElixer == "3. Super Elixer":
                playerInventory["4.Super Elixer"] = playerInventory["4.Super Elixer"] + 1
            if shopElixer == "3. Mega Elixer":
                playerInventory["6.Mega Elixer"] = playerInventory["6.Mega Elixer"] + 1
        elif elixerCost < playerGp:
            print("Not enough gold!\n")
```
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Micro review - I think you've misspelt "Elixir" throughout. \$\endgroup\$ Commented Dec 28, 2020 at 15:15
  • \$\begingroup\$ Be aware that your if shopChoice == '1', if shopChoice == '2', etc. if-blocks aren't exclusive as written; if somehow in the body of one of those blocks you accidentally modify the value of shopChoice, or if you modify any of your conditions to make them more complex and add a bug, you could wind up having more than one of those blocks executed, which I'm guessing isn't what you want, and which could be a confusing bug to encounter. I think instead you want those to be an if-elif-else chain. That way it is guaranteed to only ever execute one block. \$\endgroup\$
    – bob
    Commented Dec 28, 2020 at 20:00
  • \$\begingroup\$ The same goes anywhere else that pattern is used in your code. \$\endgroup\$
    – bob
    Commented Dec 28, 2020 at 20:02

4 Answers 4

23
\$\begingroup\$

Lets consider the design here:

The item type (weapon / armor / potion) is not relevant to the shop functionality.
All we really care about is item name, cost, and level.

You can use a namedtuple to easily create an "item" data class.

So, start by creating a list of all the items, like this:

from collections import namedtuple

Item = namedtuple('Item', ['name', 'cost', 'levels'])

gameItems = [ Item('Longsword', 20, [1]),
               Item('Greatsword', 30, [2]),
               Item('Potion', 5, [1, 2]),
               ...
               Item('Mega Elixer', 20, [5])]

Notice, that levels is a list, to allow the same item to appear in multiple store levels without creating it twice.

Now we can prepare the shop using a simple list comprehension filtering items by player level:

def get_shop_items(playerLevel):
    return [ item for item in gameItems if playerLevel in item.levels ]

You will have to do a bit more work to print available items, but not much:

def print_shop_items(shopItems):
    for index, item in enumerate(shopItems, 1):
        print(f'{index}. {item.name}: {item.cost} gp')

From here, you should be able to see how you can also eliminate the if statements for player choice, and simply pick the item from the list by index.

Then, make the player inventory a dictionary where item name is the key, and item count is the value.

A successful purchase would look something like this:

playerInventory['gp'] -= item.cost
playerInventory[item.name] += 1

This way, you can even store player currency in the inventory, instead of having a separate variable.

You may want to use Counter for this instead of a regular dictionary to avoid having extra code to handle items not yet in player inventory.

If you do care about item type at some point, like in a 'use' function, you can simply add a field called 'type' to the Item tuple, and check it in the parts of the code you care about.

You can also add a 'limit' field, if you want the player to be able to only carry certain amount of certain item, say only one sword, or up to 10 potions.

You can then check this field before adding item to the inventory and print an appropriate message if there is no room.

EDIT: I incorporated some important comments in to the answer, but I am keeping the naming convention to align with OP's code.

My thanks to everyone who helped improve this answer!

\$\endgroup\$
4
  • 2
    \$\begingroup\$ on of the notable things in python is PEP-8 for naming conventions, preferring snake_case over camelCase variables. \$\endgroup\$
    – hjpotter92
    Commented Dec 28, 2020 at 6:42
  • 1
    \$\begingroup\$ To add to the above comment, namedtuple is a class factory - a function that creates and returns a class. What you get back is a class, so should be named in CamelCase like any other class \$\endgroup\$ Commented Dec 28, 2020 at 8:46
  • 2
    \$\begingroup\$ namedtuple already existed in Python 2. And enumerate takes an optional second argument, which is the start value, so you don't need to do index + 1 if you use enumerate(items, 1). \$\endgroup\$
    – Graipher
    Commented Dec 28, 2020 at 12:56
  • 1
    \$\begingroup\$ A set may be semantically more appropriate than a list for the levels (the added performance is a side benefit) \$\endgroup\$ Commented Dec 28, 2020 at 13:20
12
\$\begingroup\$

Bug

If a player has 100 gold, and an item costs 100 gold, you generate an Error!

    if weaponCost < playerGp:
        print("You bought a", shopWeapon, "for", weaponCost, "gp!\n")
        ...
    elif weaponCost > playerGp:
        print("Not enough gold\n")
    else:  print("Error\n")

While spending every last one of the coins in your pocket may be ill-advised, the player should be allowed to do so!

You don't need an if ... elif ... else construct. If the player has sufficient gold, allow the purchase. If they don't, say not enough gold. There is no third case to worry about.

    if playerGp >= weaponCost:
        print("You bought a", shopWeapon, "for", weaponCost, "gp!\n")
        ...            
    else:
        print("Not enough gold\n")
\$\endgroup\$
4
\$\begingroup\$

This function is quite complicated. You should consider writing tests for it. For example:

If the code you gave is in a file named shop.py. Put the below code in a file called test_shop.py:

from shop import shop
from unittest import TestCase
from mock import patch


class TestBuyingThings(TestCase):
    @patch('shop.input', return_value='3')
    def test_buy_a_potion_when_inventory_empty_and_just_enough_gold(self, *args):
        playerInventory = {}
        playerGp = 20
        shop(1, playerGp, playerInventory)
        self.assertEqual(playerInventory, {'1.Potion': 1})

    @patch('shop.input', return_value='3')
    def test_buy_a_potion_when_not_enough_gold(self, *args):
        playerInventory = {}
        playerGp = 0
        with patch('shop.print') as mocked_print:
            shop(1, playerGp, playerInventory)
            mocked_print.assert_any_call("Not enough gold!\n")
        self.assertEqual(playerInventory, {'1.Potion': 1})

make sure you have mock installed pip install mock, then run the tests with python -m unittest test_shop. When I run the tests I get this output:

[staging] ~/game  $ python -m unittest test_shop
What would you like to buy?

{'1. Longsword': 20, '2. Leather Armor': 30, '3. Potion': 5, '4. Elixer': 5}
Your gp: 20
You bought a 3. Potion for 5 gp!

You now have 15 gp left

EF
======================================================================
ERROR: test_buy_a_potion_when_inventory_empty_and_just_enough_gold (test_shop.TestBuyingThings)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/mock/mock.py", line 1346, in patched
    return func(*newargs, **newkeywargs)
  File "/Users/lukebryant/game/test_shop.py", line 11, in test_buy_a_potion_when_inventory_empty_and_just_enough_gold
    shop(1, playerGp, playerInventory)
  File "/Users/lukebryant/game/shop.py", line 96, in shop
    playerInventory["1.Potion"] = playerInventory["1.Potion"] + 1
KeyError: '1.Potion'

======================================================================
FAIL: test_buy_a_potion_when_not_enough_gold (test_shop.TestBuyingThings)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/lib/python3.8/site-packages/mock/mock.py", line 1346, in patched
    return func(*newargs, **newkeywargs)
  File "/Users/lukebryant/game/test_shop.py", line 20, in test_buy_a_potion_when_not_enough_gold
    mocked_print.assert_any_call("Not enough gold!\n")
  File "/usr/local/lib/python3.8/site-packages/mock/mock.py", line 985, in assert_any_call
    raise AssertionError(
AssertionError: print('Not enough gold!\n') call not found

----------------------------------------------------------------------
Ran 2 tests in 1.606s

Both tests fail! Can you see why?

Testing like this is a very good habit to get into. Once you have a suite of tests like this you can code freely with the knowledge that if you break any existing functionality, your tests will tell you. It's worth doing purely for the sake of your own sanity, but if you wanted to apply for a job in the industry, then a tested project would also look great in your portfolio.

\$\endgroup\$
1
\$\begingroup\$

This answer is meant to be for beginners from a beginner. I try to incorporate best practices as explained by Lev M. while not changing the original structure of the code too much. I guess the most important take away is to follow the DRY principle as much as possible.

I changed the structure of the shop items to lists for easier access. The shop level then is simply an item's position in the list. The main motivation is to have a single piece of code to handle the transaction later on instead of repeating the code for each shopChoice. To keep with the initial system of shop level from 1 upwards I simply use shopLvl-1 later on.

def shop(shopLvl, playerGp, playerInventory):
    print("What would you like to buy?\n")
    shopWeapon = [["1. Longsword", 20],
                  ["1. Greatsword", 30],
                  ["1. Crested Blade", 45],
                  ["1. Diamond Sword", 75],
                  ["1. Enchanted Sword", 150]]
    shopArmor = [["2. Leather Armor", 30],
                 ["2. Chainmail Armor", 45],
                 ["2. Plate Armor", 60],
                 ["2. Diamond Armor", 125],
                 ["2. Angelic Armor", 225]]
    shopPotion = [["3. Potion", 5],
                  ["3. Potion", 5],
                  ["3. Super Potion", 10],
                  ["3. Super Potion", 10],
                  ["3. Mega Potion", 20]]
    shopElixer = [["4. Elixer", 5],
                  ["4. Elixer", 5],
                  ["4. Super Elixer", 10],
                  ["4. Super Elixer", 10],
                  ["4. Mega Elixer", 20]]

I keep the the shopItems object but turn it into a list as well. This allows for the extraction of item name and cost independent of which type of item it is. I turn shopChoice into an integer for easier use. Illegal inputs would need to be handled here.

    shopItems = [
        shopWeapon[shopLvl-1],
        shopArmor[shopLvl-1],
        shopPotion[shopLvl-1],
        shopElixer[shopLvl-1]
    ]

    print(shopItems, "\nYour gp:", playerGp)
    shopChoice = int(input("1/2/3/4"))  # TODO handle illegal inputs

    item, cost = shopItems[shopChoice-1]

I replace all the shopWeapon, etc variables and their costs with item and cost.

    if cost <= playerGp:
        print("You bought a", item, "for", cost, "gp!\n")
        time.sleep(0.8)
        playerGp -= cost
        print("You now have", playerGp, "gp left\n")
        time.sleep(0.8)

Here I keep with your initial design of playerWeapon and playerArmor variables but implement a dictionary update or create logic for potions and elixirs.

        if shopChoice == 1:
            playerWeapon = item
        elif shopChoice == 2:
            playerArmor = item
        elif shopChoice in [3, 4]:
            if item not in playerInventory:
                playerInventory[item] = 1
            else:
                playerInventory[item] += 1
            print(playerInventory)

    else:
        print("Not enough gold\n")

I also implemented AJNeufeld's bug fix regarding zero gold after purchase.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.