5
\$\begingroup\$

This is the second thing I've ever written in Python. I am just looking for what I could improve upon along with things that could be done better. It is as compact as I could make it with my current experience.

import random

def war_handler():
    war_handler.count += 1
    print "War is {0} is happening.".format(war_handler.count)

    if len(player_one) > 4 and len(player_two) > 4:
        limiter = 3
    elif len(player_one) == 4 or len(player_two) == 4:
        limiter = 2
    else:  # len(player_one) < 4 or len(player_two) < 4:
        if len(player_one) > len(player_two):
            limiter = len(player_two) - 2
        else:  # len(player_one) < len(player_two)
            limiter = len(player_one) - 2

    w, x, y, z = 0, 0, 0, 0

    for w in range(0, limiter):
        temp.append(player_one[w])
        temp.append(player_two[w])
    random.shuffle(temp)

    while x <= 4:
        player_one.remove(player_one[0])
        player_two.remove(player_two[0])
        x += 1

    if not player_one or not player_two:
        print("\n\n\n\n\n")
    else:
        if player_one[0] > player_two[0]:
            temp.append(player_one[0])
            temp.append(player_two[0])

            player_one.remove(player_one[0])
            player_two.remove(player_two[0])

            for y in range(0, len(temp)):
                player_one.append(temp[y])

        elif player_one[0] < player_two[0]:
            temp.append(player_one[0])
            temp.append(player_two[0])

            player_one.remove(player_one[0])
            player_two.remove(player_two[0])

            for z in range(0, len(temp)):
                player_two.append(temp[z])

        else:  # player_one[0] == player_two[0]:
            war_handler()


# Init important variables and get players names
player_one_name = raw_input("Whats Player one's name? ")
player_two_name = raw_input("Whats Player two's name? ")
player_one = []
player_two = []
temp = ""
i = 0

# Input the players cards!
while i <= 1:
    # Get Player one's cards
    if i == 0:
        print "Input player {0}'s cards. Note: Capitals only!".format(player_one_name)
        while temp != "END":
            player_one.append(temp)
            temp = raw_input("Card: ")

        # Remove the blank in the array
        if '' in player_one: player_one.remove('')
        i += 1
    # Get Player two's cards
    if i == 1:
        temp = ''
        print "Input player {0}'s cards. Note: Capitals only!".format(player_two_name)
        while temp != "END":
            player_two.append(temp)
            temp = raw_input("Card: ")

        # Remove the blank in the array
        if '' in player_two: player_two.remove('')
        i += 1

# Switch the alphabet to numbers 1 - 10, j, q, k , a for each deck # Switch to dictionary
translation_list = ['J', 'Q', 'K', 'A']
translation_list_1 = ['11', '12', '13', '14']
for i in range(0, len(translation_list)):
    player_one = [item.replace(translation_list[i], translation_list_1[i]) for item in player_one]
    player_two = [item.replace(translation_list[i], translation_list_1[i]) for item in player_two]

# Make the lists integers
player_one = [int(num) for num in player_one]
player_two = [int(num) for num in player_two]

total_wars = 0

# Play the game!
while player_one and player_two:
    temp = []
    # War Sim
    ###################################################################################################################
    if player_one[0] == player_two[0] and len(player_one) >= 4 and len(player_two) >= 4:
        war_handler.count = 0
        war_handler()
        total_wars += war_handler.count
        # War sim over

    # Simple determination of winner
    elif player_one[0] != player_two[0]:
        temp.append(player_one[0])
        temp.append(player_two[0])

        random.shuffle(temp)

        player_one.remove(player_one[0])
        player_two.remove(player_two[0])

        if player_one > player_two:

            player_one.append(temp[0])
            player_one.append(temp[1])

        else:  # player_one < player_two

            player_two.append(temp[0])
            player_two.append(temp[1])
    else:
        continue

if not player_one:
    print "Player {0} Wins!".format(player_two_name)
elif not player_two:
    print "Player {0} Wins!".format(player_one_name)
print "Total Wars Completed: {0}".format(total_wars)
\$\endgroup\$

1 Answer 1

6
\$\begingroup\$

Overall structure

The most important thing to improve is to decompose this code to multiple smaller functions. At the same time, remove all code in the global namespace, so that the program has this structure:

import ...

def main():
    ...

if __name__ == '__main__':
    main()

See the next section for a closely related tip.

Don't repeat yourself

This snippet is repeated twice to input cards:

print "Input player {0}'s cards. Note: Capitals only!".format(player_one_name)
while temp != "END":
    player_one.append(temp)
    temp = raw_input("Card: ")

# Remove the blank in the array
if '' in player_one: player_one.remove('')

The most common technique to reduce repeated logic is to use functions, for example:

def input_cards(player, player_name):
    print "Input player {0}'s cards. Note: Capitals only!".format(player_name)
    while temp != "END":
        player.append(temp)
        temp = raw_input("Card: ")

    # Remove the blank in the array
    if '' in player:
        player.remove('')

Then you could call this with for each player:

input_cards(player_one, player_one_name)

input_cards(player_two, player_two_name)

Improving the input of cards

The current method of inputting cards could use some improvement:

  • Instead of removing blanks, it would be better to never add any
  • Instead of converting input values to numbers later, it would be better at input time
  • You should validate input here rather than later, so that the rest of the program can trust the input data without worrying about possibly invalid values
  • The loop condition is evaluated pointlessly when it runs for the first time. It would be better to buy convert it to a while True loop.
  • The message "Note: Capitals only!" is unclear. I suppose you meant that users should enter "END" with all capitals to end the input

Applying the above suggestions the method becomes:

def input_cards(player, player_name):
    print("Input player {0}'s cards. To stop, enter END with all capitals!".format(player_name))
    while True:
        temp = raw_input("Card: ")
        if temp == "END":
            break
        try:
            player.append(int(temp))
        except ValueError:
            print('Not a number: {0}'.format(temp))

Inputting the cards of the players

This is the way you input the cards of the players and it's very strange:

# Input the players cards!
while i <= 1:
    # Get Player one's cards
    if i == 0:
        # ...
        i += 1

    # Get Player two's cards
    if i == 0:
        # ...
        i += 1

With the helper methods developed in the earlier section, all that code can be reduced to:

# Input the players cards!
input_cards(player_one, player_one_name)
input_cards(player_two, player_two_name)

Next steps

Look for other opportunities to extract logic to more, smaller functions. You should be able to gradually simplify and reduce the size of the code.

\$\endgroup\$

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.