8
\$\begingroup\$

I am creating a prototype for a text-adventure to get into game development, and i wanted to take the opportunity to also try to use OOP concepts, since i have been struggling to understand the OOP design.

Here is my code so far (warning of funky names):

class Character:
    def __init__(self, name, sex):
        self.name = name
        self.sex = sex
        self.inventory = {
            "albums": [],
            "instruments": []
        }
        self.coordinates = {
            'n': 0,
            'e': 0,
            's': 0,
            'w': 0
        }

    def get_inventory(self):
        print(self.inventory)

    def get_coordinates(self):
        print(self.coordinates)

    def str_coordinates(self):
        to_string = [str(v) for k, v in self.coordinates.items()]

        return ' '.join(to_string)

    def __str__(self):
        return  self.name

class Map:
    def __init__(self, *args):
        self.rooms = [*args]

    def get_room(self, room_coordinates):
        current_room = ''

        for room in self.rooms:
            if room.coordinates == room_coordinates:
                current_room = room

        return current_room

    def get_rooms(self):
        rooms_str = ''
        for room in self.rooms:
            rooms_str += room.coordinates + "\n"

        return rooms_str

class Room:
    def __init__(self, coordinates, name, actions, items, next_rooms):
        self.coordinates = coordinates
        self.name = name
        self.actions = actions
        self.items = items
        self.next_rooms = next_rooms

    def str_actions(self):
        return ', '.join(self.actions)

    def get_items_types(self):
        items_types = [item.item_type for item in self.items]
        return items_types

    def get_items_names(self):
        items_names = [item.name for item in self.items]
        return items_names

    def __str__(self):
        return self.name

class Item:
    def __init__(self, item_type, name):
        self.item_type = item_type
        self.name = name

class Album(Item):
    def __init__(self, item_type, name, lyrics):
        super().__init__(item_type, name)
        self.lyrics = lyrics

    def __str__(self):
        return self.item_type

ttng_animals = Album("Album", "Animals", 
                     "Let's talk about facts, the very best moment we have")

math_rock_3 = Album("Album", "Math rock 3", 
                    "Doesn't__init__ know the lyrics, play awesome guitar solo with tapping")

sort_of = Album("Album", "A test", 
                    "Doesn't__init__ know the lyrics, play awesome guitar solo with tapping")

standard_actions = ["look", "move", "coordinates", "inventory", "take"]

player = Character("Mabuelalelelejando", "male")

start = Room("0 0 0 0", "Start", standard_actions, [math_rock_3, ttng_animals, sort_of], ['n'])
room_2 = Room("1 0 0 0", "Room 2", standard_actions, [("instrument", "nya")] , ['s'])

game_map = Map(start, room_2)

def update_north():
    if player.coordinates['s'] > 0:
        player.coordinates['s'] -= 1
    else:
        player.coordinates['n'] += 1

def update_east():
    if player.coordinates['w'] > 0:
        player.coordinates['w'] -= 1
    else:
        player.coordinates['e'] += 1

def update_south():
    if player.coordinates['n'] > 0:
        player.coordinates['n'] -= 1
    else:
        player.coordinates['s'] += 1

def update_west():
    if player.coordinates['e'] > 0:
        player.coordinates['e'] -= 1
    else:
        player.coordinates['w'] += 1

def update_coordinates(movement):
    if movement.lower() == 'n':
        return update_north()
    if movement.lower() == 'e':
        return update_east()
    if movement.lower() == 's':
        return update_south()
    if movement.lower() == 'w':
        return update_west()

def ask_for_action():
    current_room = game_map.get_room(player.str_coordinates())
    print(current_room.str_actions())
    action_decision = input('').split()

    while action_decision[0] not in current_room.actions:
        print(current_room.str_actions())
        action_decision = input('').split()

    return action_decision

def move(*args):
    current_room = game_map.get_room(player.str_coordinates())
    directions = current_room.next_rooms

    print("You can move to: {}. A direction alsjeblieft:".format(', '.join(directions)))
    direction = input('')

    while direction.lower() not in directions:
        print("{}. A direction alsjeblieft:".format(directions))
        direction = input('')

    update_coordinates(direction)

def take(*args):
    current_room = game_map.get_room(player.str_coordinates())
    items_types = list(map(str.lower, current_room.get_items_types()))
    items_names = list(map(str.lower, current_room.get_items_names()))

    separated_items_names = []

    for item_name in items_names:
        for word in item_name.split():
            separated_items_names.append(word)

    if args:
        arg_in_name = []
        arg_not_in_name = set()
        for arg in args:
            if arg in separated_items_names:
                arg_in_name.append(arg)
            else:
                arg_not_in_name.add(arg)

        print(arg_in_name, arg_not_in_name)

        if len(arg_not_in_name) > 0 and len(arg_in_name) > 0:
            for i in range(0, len(items_names)):
                if arg_in_name[0] in items_names[i]:
                    item_index = i
            print("Do you want to take the {}?".format(items_names[item_index]))

        if len(arg_not_in_name) == 0 and len(arg_in_name) > 0:
            asked_items = set()
            for i in range(0, len(items_names)):
                for name in arg_in_name:
                    if name in items_names[i]:
                        asked_items.add(items_names[i])
                        item_index = i

            if len(asked_items) > 1:
                print("Do you want to take the {}?".format(items_names[item_index]))
            else:
                player.inventory["albums"].append(current_room.items[item_index])
                player.get_inventory()

actions = {
    "move": move,
    "take": take,
    "inventory": player.get_inventory,
    "coordinates": player.get_coordinates
}

def main():

    while True:
        action = ask_for_action()
        if action[0] in actions:
            args = action[1:]
            if len(args):
                actions[action[0]](*args)
            else:
                actions[action[0]]()
        else:
            print("Sorry please, alsjeblieft ):")

main()

Please, let me know about any way you think this can be further improved. I haven't introduced things like puzzles, so items are not very useful. I just implemented the take function.

Also, any suggestion to improve the OOP design on this game, as well as my understanding in OOP will be very appreciated.

\$\endgroup\$
  • 1
    \$\begingroup\$ Jus a suggestion, but if you're going beyond this, you might want to take a look at the cmd.Cmd class from the standard library. \$\endgroup\$ – D Krueger Feb 11 '18 at 21:34
  • \$\begingroup\$ Thank you for the suggestion Krueger! I need to research about that, but i guess it has to do with text and terminal programs. Could you give me an example? \$\endgroup\$ – ManuAlvarado22 Feb 11 '18 at 21:37
  • 2
    \$\begingroup\$ +1 for the use of TTNG \$\endgroup\$ – socrates Feb 11 '18 at 22:01
  • \$\begingroup\$ Hey, you are such a nice guy! It probably has something to do with your name ;) Funnily enough, that lyrics fragment is not in the Animals album, but it is such a good song. \$\endgroup\$ – ManuAlvarado22 Feb 11 '18 at 22:05
6
\$\begingroup\$

Let's start from some small comments about your code :)

Character class

  • get_invetory, get_coordinates - When you create a class and you want to have access to some private fields (we can say that your inventory field is something like a private field), then a popular technique is to create functions which are known as getter and setter. The typical convention is naming this function set_x/setX and get_x/getX. In your code, you created something like a getter (because you named this get_invetory), but this is not a normal getter (because you return nothing). Also there is another thing, in python creating getter/setter methods in the classic way is not so popular, because classes in python don't have something like private fields (maybe this is not exactly true, but this is not important now). Using fields of an object directly is not a problem (in common cases). So, I think that you should change get_invetory to either return something or maybe try to rename these methods.

class Map

  • In the constructor, you convert from tuple to list. Why? Your code doesn't need that. This is not a bug/error, but just a bad code smell.

  • get_room should return None when it find nothing, not the empty string.

class Room

  • This is also not a bug, but I think that changing get_items_types to a one liner would improve the readability of this code.

Now, let's talk about how to improve this code to be more OOP.

The strength of Object Oriented Programming is not the fact that you can collect some variables in one object. The true power is that you can use polymorphism. In your code there is not a lot of use of that (because you use inheritance only once), so you don't get the big advantages of using OOP in this program.

But there is another thing.

The methods which change state of some object should be part of that object. For example, methods like update_north should be inside of the character class (because this is part of the implementation of moving your character). This is a great example of a method which should be part of a class. You should do the same for the take and move methods. Putting this method into the character class, you will be able to use self syntax, which will be very useful if you want to start using polymorphism in your code. Also you will be able to create more than one player and be able to move this character. Using a global variable like player is not good and should be the first thing to fix if you want to make this code more OOP.

Also:

  • Missing unit tests ! ;) It is a very very important thing! Good units tests will save your time when you will be debugging or, what is more important, refactoring your code.

  • Start using the logging module, not print :)

  • If I correctly understand you code, you don't need 4 coordinates. You need only x and y :)

  • The take method is too long and too complicated, in my opinion. Try to split this method into some smaller methods to improve readability.

\$\endgroup\$
  • \$\begingroup\$ Hello Mariusz! First of all, thank you so much for such an insightful review of my code, i really really appreciate that you have taken the time to read through the whole code and give so many suggestions. I feel sort of the same about those methods which name start with get, i am aware of getters and setters, but i don't use them myself because i don't really know when/why/how. So, do you think a change in the name would be enough? What name would you suggest? In the constructor of the class Map, i'm not sure what do you mean with converting from tupple to list, but i guess it is... \$\endgroup\$ – ManuAlvarado22 Feb 11 '18 at 19:38
  • \$\begingroup\$ Continue: Because of the *args. I sort of wanted to try *args and i found it to be a good opporunity. So i should not wrap the *args in a list? Oh, before i forget it, you mentioned that i didn't return anything. I am having some troubles with that, i do like to return things, i am more of a JavaScript guy so one tend to use sooome functional programming there. But if i return it, i then have to print it and i think there is something wrong/not optimal in the implementation that doesn't allow me to return those. I think it is the way i am handling the actions, with the dict. \$\endgroup\$ – ManuAlvarado22 Feb 11 '18 at 19:46
  • \$\begingroup\$ Continue 2: About the class Room, i'll make that one-liner! OOP: Yes, the thing is that i don't really understand quite well the philosophy behind OOP, now is when i am just understanding how i could see everything as an object. Could you give me some use cases of Polymorphism? Also, the only class that inherits from another (Which is Album so far) sounds redundant because it inherits the type attribute, which is Album, just the name of it. Is it alright? In regards to turning move and take into methods, yes, it is something i have been really considering, and it actually upsets me... \$\endgroup\$ – ManuAlvarado22 Feb 11 '18 at 19:49
  • \$\begingroup\$ Continue 3: That they are currently global functions. But i do not understand how to make them methods, because i don't know how i could get the current room, for example, if i would be refferencing the Room class before creating it. What could i do? About unit tests, yes! I see how tests would be great for this game, and it would be a way to actually start understanding it. I have just tried some unit testing with the Django built-in tests, but i don't even use Django anymore, so i would need to make some research. I would need to try loggin them. Oh, hopefully i do need those... \$\endgroup\$ – ManuAlvarado22 Feb 11 '18 at 19:53
  • \$\begingroup\$ Continue 4: Four coordinates, but just to not make things to complicated, i am using just two rooms. About the take function, yes, i agree with you. I don't like seeing it being so long either, but it i only have Zork as a reference, because i can play it online, and i liked the way they handle taking items, because you don't have to type the whole item name, but only a part of it. I would have to check how i could make it more readable. Thank you so much again for such an incredible review! It really helped me a lot. \$\endgroup\$ – ManuAlvarado22 Feb 11 '18 at 19:56

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.