4
\$\begingroup\$

I am making an RPG game using python. At the moment, I have programmed choosing your character and saving it.

However, today I previously asked a question NameError: name 'character' is not defined on stackoverflow. Whilst I got my question answered and my program is now working, I received a comment on my question (from a person with 15k reputation so I am guessing they can be trusted) stating:

#BTW: Once you have it fixed, take your code to codereview.stackexchange.com, there are a few very bad practices in there

I'm guessing the person was referring to my code, so i have come over here to get some feedback.

Whilst I am a beginner and I am unsure how to improve on any bad practices I have made, I know that criticism is only a good thing and I can learn from my mistakes. I was wondering if someone could point out anything/give me some tips on what I could do better with my program. Thanks.

My full code:

from os import system
from ascii_art_functions import mountain_range
from ascii_art_functions import character_selection_horse_and_knight
from ascii_art_functions import screen_line

import random
import math
import time
import datetime
import pickle
import sys
import os

#miscellaneous functions + procedures

def w(t):
    time.sleep(t)

def version_counter(filename="adventure_colussus_version_counter.dat"):
    with open(filename, "a+") as f:
        f.seek(0)
        val = int(f.read() or 0) + 1
        f.seek(0)
        f.truncate()
        f.write(str(val))
        return val
counter = version_counter()

def t(text):
    for char in text:
        sys.stdout.write(char)
        sys.stdout.flush()
        time.sleep(0.021)

def save_character(save_name, character):
    save_name_pickle = save_name + '.pickle'
    t('> saving character')
    w(1)
    with open(save_name_pickle, 'wb') as f:
        pickle.dump(character, f)
        t('> character saved successfully')

def load_character(load_name):
    load_name_pickle = load_name + '.pickle'
    t(' > loading character...')
    w(1)
    pickle_in = open(load_name_pickle,"rb")
    character = pickle.load(pickle_in)
    t(' > character loaded successfully\n')
    t('\n> welcome back ')
    t(character['name'])
    t('!!!\n')
    w(0.5)
    t('\n> here are your stats from last time: \n')
    print(' >',character)



def character_generator():
    t('\n > we have reached the first crucial part of your journey: we must choose the path that you will take! the decision\n')
    w(0.5)
    t(' > is up to you my friend! Whether you choose to be a bloodthisty warrior or a cunning and strategic fighter,\n')
    w(0.8)
    t(' > the choice is yours!\n')
    w(0.5)
    t('\n > now then, lets get right into it!')
    w(0.8)
    t(' are you more of a tanky player[1] or a strategic player[2]?')
    
    player_t_choice_1 = input('\n > ')

    if player_t_choice_1 == '1':
        health = 100
        damage = 75

    elif player_t_choice_1 == '2':
        health = 75 
        damage = 50

    t('\n > so, we have that out of the way, lets carry on!')
    w(0.8)
    t(' oh of course! sorry to forget! another quick question: ')
    w(0.5)
    t('do you like \n')
    t(' > to use magic from great distances[1] or run in to the thick of battle weilding a deadly blade[2]? ')

    player_t_choice_2 = input('\n > ')

    if player_t_choice_2 == '1':
        shield = 50
        magic = 75

    elif player_t_choice_2 == '2':
        shield = 75 
        magic = 45

    t('\n > good good! we have decided your play style and your preffered ways of attacking the enemy!\n')
    w(0.5)
    t(' > now, we must see what luck we are able to bestow upon you. be warned: it is entirely random!\n')
    w(0.8)
    random_luck = input('\n > press enter to roll a dice...')
    w(0.3)
    t(' > rolling dice...\n')
    luck = random.randint(0,10)
    w(1)
    print(' > your hero has',luck,'luck out of 10!')
    w(0.8)
    t('\n > we have reached the most important part of creating your character! The naming!\n')
    t(' > choose wisely. your hero will be named this for the rest of their lives...\n')
    w(1)

    t('\n > what should your hero be named?\n ')
    name = input('> ')
    w(1)
    t('\n > welcome mighty hero! you shall be named: ')
    t(name)
    t(' !!!\n')
    t(' > a fine choice')
    t('\n')

    t(' \n > now then. i guess you be on your way! you have a journey to start and a belly to fill!\n')
    t(' > i have to say, i have rather enjoyed your company! feel free to come by at any time!\n ')
    t('> goodbye and god speed!')
    w(1)
    print('\n')
    t(' > your final stats are as follows: \n')
    w(0.3)
    print(' >', '[', 'health:', health, ', damage:',  damage, ', shield:', shield, ', magic:', magic, ', luck:', luck, ', name:', name, ']')
    t('\n > we should now save your character if you want to come back to it later:\n ')

    character = {'health': health, 'damage': damage, 'shield': shield, 'magic': magic, 'luck': luck, 'name': name}
    character_file_name = input('> ')
    save_character(character_file_name, character)



# main game        

w(0.5)
e = datetime.datetime.now()
screen_line()
print('\n  <Adventure Colussus>         version: v',counter,'| current date: ',e, '| date of creation: 9.2.2021')
screen_line()
w(0.5)
mountain_range()
screen_line()
print('\n > [1] create new game')
print(' > [2] load existing game')
print(' > [3] end game')
print(' > [4] credits')
choice = input("\n > ")

if choice == '1':

    t("\n > you have chosen to create a new game: redirecting...")
    w(0.75)
    system('cls')
    screen_line()
    print('  \n  we will begin with creating your character:                                        quick tip: choose wisely')
    screen_line()
    w(0.5)
    character_selection_horse_and_knight()
    screen_line()
    w(0.3)
    
    character_generator()




elif choice == '2':

    t("\n > you have chosen to load an existing game: redirecting...")
    w(0.75)
    system('cls')
    w(0.5)
    screen_line()
    print('  \n  we will begin with choosing an existing character:                             quick tip: make sure it exists!')
    screen_line()
    w(0.5)
    character_selection_horse_and_knight()
    screen_line()
    

    character_file_name = input('\n > character file name: ')
    load_character(character_file_name)



elif choice == '3':

    t(' > ending session...')
    w(0.5)
    t(' > session ended successfully')
    w(1)
    sys.exit()

elif choice == '4':
    pass

else:
    t('incorrect response. please try again')
    
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Welcome to Code Review! This question lacks any indication of what the code is intended to achieve. To help reviewers give you better answers, please add sufficient context to your question, including a title that summarises the purpose of the code. We want to know why much more than how. The more you tell us about what your code is for, the easier it will be for reviewers to help you. The title needs an edit to simply state the task. \$\endgroup\$ Feb 18 at 22:10
  • 2
    \$\begingroup\$ well i have edited it now a little! i hope that helps! \$\endgroup\$ Feb 18 at 22:31
3
\$\begingroup\$

Naming

I would make the following changes

w() -> remove
t() -> print_text()

Since w() is just time.sleep, it isn't necessary. And someone looking at your code would understand time.sleep a lot after than seeing w(0.08), and having to look to where w is defined.

print_text is a lot more descriptive than just t.

Docstrings

Adding a simple doctoring inside each function can help immensely when you need a quick reminder of what a function does. For example,

def version_counter() -> int:
    """
    Determines the version of the last played game, either in the {VERSION_FILENAME}
    file, or generating a new file if none is found.
    """

Type Hints

You can add type hints to display what types of parameters are accepted, and what types are returned, if any. For example,

from typing import Dict, Any
def save_character(save_name: str, character: Dict[str, Any]) -> None:
    """
    Saves the current character to a pickle database.
    """

Before, seeing just character, one might assume it's just a string representing the characters name. Now, it's clearly represented as a dictionary with string keys and values that can be any type.

The most important distinction here is making sure someone reading your code knows the types accepted by your function, and the types returned. Since this function doesn't "return" a value, you use None.

Imports

You don't need import os, because in your program you only use the system function you imported a few lines above. You also don't need math.

Less Code

I notice you time.sleep after almost every time you print to the console. Instead, I would recommend passing that value to the (now named print_text function), and do the time.sleep in there. For example,

def print_text(text: str, sleep_time: float=0.0) -> None:
    """
    Prints the text to the console character by character. RPG style.
    """
    for char in text:
        sys.stdout.write(char)
        sys.stdout.flush()
        time.sleep(0.021)
    if sleep_time != 0.0:
        time.sleep(sleep_time)

Then you use it as such:

print_text('> saving character', 1)

And if you don't want to wait after, since sleep_time is a default parameter, just

print_text('> saving character')

Error Checking

I'm playing your game, reach the character generator function, and accidentally press 3 for the first question! Now your code silently fails, my health and damage aren't set, and I don't know what's wrong later on. I would recommend something like this:

def get_input(string: str, valid_options: List[str]) -> str:
    while True:
        user_input = input(string)
        if user_input in valid_options:
            return user_input

Then used like this:

print('\n > [1] create new game')
print(' > [2] load existing game')
print(' > [3] end game')
print(' > [4] credits')
choice = get_input("\n > ", ['1', '2', '3', '4'])

Consistent Spacing

In some spaces in your code, there's one space between if/else statements, and other places there's 3 or 4 spaces. Being consistent can help organize your code (my recommendation would be to stick with less spaces between statements)

Obligatory f""

Using f-strings allows you to directly include variables in your strings. For example, the below line

print(' >', '[', 'health:', health, ', damage:',  damage, ', shield:', shield, ', magic:', magic, ', luck:', luck, ', name:', name, ']')

can be condensed to

print(f" > [ health: {health}, damage: {damage}, shield: {shield}, magic: {magic}, luck: {luck}, name: {name} ]")
\$\endgroup\$
1
  • \$\begingroup\$ Thanks a ton! It looks like you have put a lot of effort into your answer and you have pointed out several things, which i will definetly improve on. Thanks again! \$\endgroup\$ Feb 19 at 9:00

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.