3
\$\begingroup\$

New to programming with the goal being to create my own roguelike. I am mostly interested right now in creating my item generation system. I want the code to create a random item that can then be used, stored in an inventory etc. Want to know if the below is a smart way to begin with. Any suggestions to make the below better would be much appreciated.

The item description within function is just there as placeholder so I can see if the code was working. Will this object be permanent ? IE everytime I call create_item it will create a permanent object - Sorry for terminology and if incorrect legit second week of playing with Python :)

import random

i_quality = ["Good", "Bad"]
i_quality = random.choice(i_quality)
i_base = ["Sword", "Gun"]
i_base = random.choice(i_base)
i_element = ["Fire", "Water"]
i_element = random.choice(i_element)

class Item(object):
    def __init__ (self, quality, base, element):
        self.quality = quality
        self.base = base
        self.element = element

def create_Item():
    new_item = Item(i_quality, i_base, i_element)
    i_description = (new_item.quality + " " + new_item.base + " of " + new_item.element)
    print (i_description)

create_Item()
\$\endgroup\$
  • \$\begingroup\$ Just note, the last bit that you added isn't relevant for a Code Review (yes, these sites are very strict on what's on topic). The answer though is no: A new new_item will be created when the function enters, and destroyed (some time after) the function exits. You need to return it or store it somewhere if you want to use it. \$\endgroup\$ – Carcigenicate Jul 4 at 1:22
3
\$\begingroup\$

This isn't bad code for someone learning the language, but there are a few things I'd change:

At the top you have:

i_quality = ["Good", "Bad"]
i_quality = random.choice(i_quality)

And other such lines. The main problem here is you're overwriting i_quality. What if you wanted to generate a second quality later? You overwrote your master list of qualities. Have them as separate variables:

all_qualities = ["Good", "Bad"]
all_bases = ["Sword", "Gun"]
all_elements = ["Fire", "Water"]

i_quality = random.choice(all_qualities)
i_base = random.choice(all_bases)
i_element = random.choice(all_elements)

Be careful how you name things and what the responsibilities of functions are. create_Item does create an Item, but it also prints it out then discards it. Such a function isn't very useful. The caller could have just called the Item constructor themselves and then printed the object if they wanted to. Your function does some formatting, but that would be better off in the __str__ method of Item.

What would make for a useful factory function though would be a create_random_item function that returns a random Item. Taking all that together, I'd change this code to something closer to:

import random

all_qualities = ["Good", "Bad"]
all_bases = ["Sword", "Gun"]
all_elements = ["Fire", "Water"]

class Item(object):
    def __init__ (self, quality, base, element):
        self.quality = quality
        self.base = base
        self.element = element

    # What Python will call when you ask it to
    #  display an Item using str
    def __str__(self):
        return self.quality + " " + self.base + " of " + self.element

def create_random_item():
    quality = random.choice(all_qualities)
    base = random.choice(all_bases)
    element = random.choice(all_elements)

    return Item(quality, base, element)

for _ in range(10): # We'll create 10 random weapons and print them out
    item = create_random_item()
    print(str(item)) # str calls item's __str__

Good Gun of Fire
Good Sword of Fire
Bad Sword of Water
Good Sword of Fire
Good Sword of Water
Good Sword of Fire
Bad Gun of Water
Bad Sword of Water
Bad Gun of Water
Bad Sword of Water

  • This could be further improved though. Any time you have a closed set of members of a set (like how "fire" and "water" are members of the "elements" set), you should likely being using an Enum. They help avoid errors in many cases, and allow IDEs to assist you in auto-completing names.

  • In __str__, you could also make use of f-strings to avoid needing to manually concat using +. It would be neater to write:

    def __str__(self):
        return f"{self.quality} {self.base} of {self.element}"
    

    That allows for much less noise and allows you to focus more on what you what printed, and less on dealing with opening and closing quotes and +s.

  • For the sake of context, create_random_item may make more sense as a static method. It makes it clearer what class it's associated with, since then you'd call the method as:

    Item.create_random_item()
    
\$\endgroup\$
  • \$\begingroup\$ My friend most helpful thank you - I love how everything seems to be a giant logic puzzle :) I really appreciate the pointers back to work. I had no idea __str__(self) allows you to return a string! amazing \$\endgroup\$ – Acidreign Jul 4 at 1:55

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.