4
\$\begingroup\$

I just want some opinions/tips on how to improve.

#import 
import string
import random
import time
import pickle


#create grid
def create_grid(size,lastcell,numberofmines):
    grid = []
    for i in range(size):
        row = ['0']*size
        grid.append(row)
    mines = create_mines(grid,lastcell,numberofmines,size)
    p = surrounding(grid,size)
    p.numberofsurrounding(grid,size)
    return (grid,mines)


#show the grid 
def showgrid(grid,size):
    horizontal = ' -'+size*'----'
    collum = '   '
    #writes the collmum numbers
    for i in string.ascii_uppercase[:size]:
        collum += (i+ '   ')
    print (collum,'\n',horizontal)
    # writes row numbers
    for idx,i in enumerate(grid, start=1):
        row = str(idx)
        row += '|'
        for j in i:
            row = row+' '+j+' |'
        print (row+'\n'+horizontal)


#generated random cordinates 
def generate_cordinate(size):
    a = random.randint(0,size-1)
    b = random.randint(0,size-1)
    return (a,b)


#class fro surrounding
class surrounding(object):
    def __init__(self,grid,size):
        self.grid = grid
        self.size = size


    #creates a list with the surrounding cell for every cell
    def surrounding_cells(self,row_num,col_num,size):
        surronding = []
        for i in range(-1,2):
            for j in range(-1,2):
                if i == 0 and j == 0:
                    continue
                elif -1<row_num+i<size and -1<col_num+j<size:
                    surronding.append((row_num+i,col_num+j))
        return (surronding)


    #checks how many is mines in the surromding cells
    def numberofsurrounding(self,grid,size):
        for row_num,row in enumerate(grid):
            for col_num,col in enumerate(row):
                if col!='*':
                    #finds value of surrounding cell
                    values = [grid[r][c] for r,c in self.surrounding_cells(row_num, col_num,size)]
                    # counts how many are mines
                    grid[row_num][col_num] = str(values.count('*'))


# Generate mines
def create_mines(grid,lastcell,numberofmines,size):
    mines = []
    for i in range(numberofmines):
        cell = generate_cordinate(size)
        while cell==(lastcell[0],lastcell[1]) or cell in mines:
            cell = generate_cordinate(size)
        mines.append(cell)
    for i,j in mines: grid[i][j] = '*'
    return mines


#"shows the choose cell
def showcell(grid,showngrid,row_num,col_num,size):
    #if you pick already shown cell
    if showngrid[row_num][col_num]!='-':
        return
    #shows the cell
    showngrid[row_num][col_num] = grid[row_num][col_num]
    #if the cells value is 0 controll nearby cells
    if grid[row_num][col_num] == '0':
        p = surrounding(grid,size)
        for r,c in p.surrounding_cells(row_num,col_num,size):
            showcell(grid,showngrid,r,c,size)


# replay
def replay():
    val = input('What to go to mainmenu?(yes or no):')
    if val.lower() == 'yes':
        mainmenu('mainmenu')
    elif val.lower() == 'no':
        print('bye')
        quit()
    else:
        print('\nonly yes or no')
        replay()


#function for flags
def putflag(showngrid,row_num,col_num,flags):
    # adds flag
    if showngrid[row_num][col_num]=='-':
        showngrid[row_num][col_num] = 'F'
        flags.append((row_num,col_num))
    #remove flag
    elif showngrid[row_num][col_num]=='F':
        showngrid[row_num][col_num] = '-'
        flags.remove((row_num,col_num))


#function for picking the size/ number of mines 
def pickvalues():
    size = goodvalues(4,10,message= '\nPick size of grid(4-9):')
    numberofmines = goodvalues(size, ((size**2) -5), message= 'the number of mines has to be between ' + str(size) + ' and '+ str((size**2-6))+ '\nhow many mines::' )
    return size,numberofmines


#cheecks the values, (size/numberofmines)
def goodvalues(min,max,message):
    a = False
    while a == False:
        try:
            värde = int(input(message))
            if värde not in range(min,max):
                a = False
            if värde in range(min,max):
                a = True
        except ValueError:
            print("\nchoose an interger\n")
            a = False
    return (värde)

def name(message):
    username = input(message)
    if len(username) <= 0 or len(username) >10:
        name('Pick a username:')
    return username


#mainprogramme
def play():
    username = name('pick a username(1-10signs):')
    size,numberofmines = pickvalues()
    start_time = time.time()
    showngrid = [['-' for i in range(size)] for i in range(size)]        #skapar en kopia av spelplanen utan mines
    showgrid(showngrid,size)
    first_round = True
    flags = []
    while True:
        while True:
            flag = False
            lastcell = input('pick cell: ')
            try:
                if lastcell[2].lower() == 'f':
                    flag = True
            except IndexError:
                pass
            try:
                #Gör om kordinaterna till siffror
                lastcell = (int(lastcell[1])-1,string.ascii_lowercase.index(lastcell[0].lower()))
                break
            except (IndexError,ValueError):
                showgrid(showngrid,size)
                print ("cant choose that cell")

        #skapar spelplanen efter första rundan
        if first_round == True:
            first_round = False
            grid,mines = create_grid(size,lastcell,numberofmines)

        row_num = lastcell[0]
        col_num = lastcell[1]

        #lägger flag
        if flag == True:
            putflag(showngrid,row_num,col_num,flags)

        else:
            try:
                if grid[row_num][col_num] == '*':
                    result('Game Over',grid,showngrid,start_time,numberofmines,size,username)

                else:
                    showcell(grid,showngrid,row_num,col_num,size)
            except IndexError:
                print('\ncant choose that cell\n')

        #kcheeks for victory
        controll(grid,showngrid,size,numberofmines,start_time,username,flags,mines)

        showgrid(showngrid,size)

#kfunction to check fo victory
def controll(grid,showngrid,size,numberofmines,start_time,username,flags,mines):
    empty_cells = 0
    cellswithmines = 0
    for x in range(len(showngrid)):
        y = (showngrid[x].count('-'))
        empty_cells += y
    for x in range(len(grid)):
        z = (grid[x].count('*'))
        cellswithmines += z
    if empty_cells == cellswithmines or set(flags) == set(mines):
        result('YOU WON',grid,showngrid,start_time,numberofmines,size,username)


#shows the reuslt (lose/win)
def result(result,grid,showngrid,start_time,numberofmines,size,username):
    if result == 'Förlust':
        print('Game Over!')
        showgrid(grid,size)
        replay()
    if result == 'Vinst':
        print('*********** YOU WON! ************')
        showgrid(showngrid,size)

        score(start_time,numberofmines,size,username)


# calculates the score
def score(start_time,numberofmines,size,username):
    finish_time = time.time()
    time = start_time - finish_time
    user_score = (int((1000 * ((int(numberofmines))**2 / (int(size))) - (1.5 * time))), username)
    highscore(user_score)


#Opens highscore and adds you on the list (if youre good enough)
def highscore(user_score):
    with open ('high_scorelist.dat','rb') as file:
         high_scorelist = pickle.load(file)
    if user_score[0] > high_scorelist[len(high_scorelist)-1][0]:
        print ("YOU MADE IT!\n\n")
        high_scorelist.append(user_score)
        high_scorelist.sort(reverse=True)
        del high_scorelist[len(high_scorelist)-1]
        with open('high_scorelist.dat','wb') as file:
            pickle.dump(high_scorelist,file)
        show_highscore(high_scorelist)
        return (high_scorelist)
    else:
       show_highscore(high_scorelist)


#prints the highscore table
def show_highscore(high_scorelist):
    print('****highscore***\n\n')
    print('|        name        |    score    |')
    for i in range(10):
        print('| ',high_scorelist[i][1],' '*(17-len(high_scorelist[i][1])), end='|')
        print(high_scorelist[i][0], ' ' *(12- len(str(high_scorelist[i][0]))), end= '|\n')
    replay()


#mainmenu
def mainmenu(message):
    print(message)
    a = str(input('1. Helpmenu\n2. play\n3. Highscore\n4. quit'))
    if a == '1':
        help()
    elif a =='2':
        play()
    elif a =='3':
        highscore(user_score=(0,'test'))
    elif a == '4':
        print('Hej då!')
        quit()
    else:
        mainmenu(message='choose 1-4')


#helpmenu
def help():
    print('what can i help you with?')
    while True:
        try:
            x = int(input('1. bla\n2. blar\n3. bla\n4. play'))
            if x == 1:
                a = input(('bla '))
            elif x == 2:
                a = input(('bla'))
            elif x == 3:
                a = input('bla')
            elif x == 4:
                mainmenu(message='here we go')
        except ValueError:
            print('\nchoose\n')


#opens mainmenu
mainmenu(message='MINeSWeeper!')
\$\endgroup\$
3
\$\begingroup\$

I tried playing your game, and neither in Python 2 (after some fiddling), nor in Python 3 can I get the game logic to work properly. So it does have some bugs here and there!

Naming and spacing

  • Add space after comma and around operators in your code – This would open up your code, and make it clearer to understand.
  • Use snake_case for variable and function names – Don'tjameverythingtogether! You don't do when writing normally, don't do it when you code either. create_grid() is good, but not showgrid(), numberofsurrounding() and so on... Be consistent is the key here!
  • Add vertical spacing around code blocks - I tend to insert blank lines after for and while loops and if statements, to help separate the logical parts of the code
  • Use docstrings to describe the purpose of the function - Instead of having a comment in front of the function, use docstrings which is both according to style and helps modern IDEs to provide some help on your functions.

So instead of your original:

#create grid
def create_grid(size,lastcell,numberofmines):
   grid = []
   for i in range(size):
       row = ['0']*size
       grid.append(row)
   mines = create_mines(grid,lastcell,numberofmines,size)
   p = surrounding(grid,size)
   p.numberofsurrounding(grid,size)
   return (grid,mines)

Applying these four comments to your first function create_grid() results in:

def create_grid(size, lastcell, number_of_mines):
    """Initialize the grid with mines, and return grid and mine posistions."""

    grid = []
    for i in range(size):
        row = ['0']*size
        grid.append(row)

    mines = create_mines(grid, lastcell, number_of_mines, size)
    p = surrounding(grid, size)
    p.number_of_surrounding(grid,size)
    return (grid, mines)

Improve text handling

You do loads of string concatenation using +, which isn't really a good way to build strings. You are better off using string.format, and make patterns for entire lines at the time.

Here is an example on how to do this in show_grid(grid, size):

def show_grid(grid, size):
    """Print the full grid with headers and dividers."""

    horizontal_line = ' -{}'.format('-----'*size)    
    ROW_PATTERN = ' {:>} | {} |'

    print('     {}'.format('   '.join(string.ascii_uppercase[:size])))  
    print(horizontal_line)

    # Write rows
    for idx, gridrow in enumerate(grid, start=1):
        print(ROW_PATTERN.format(idx, ' | '.join(gridrow)))
        print(horizontal_line)

No loosing or winning

In the code as it stands there isn't any possiblity to win or loose, but this is due to editing when posting to Code Review. But it does illustrate a good point on magic number or words. In your code you had a lot of comments in Swedish, but you also used Förlust (i.e. "Game over" or "Loss") and Vinst (i.e. "Victory") as state indicators in result(). The only thing is, you translated these into English in play() and controll(), but not in result().

Ergo it is not possible to win or loose in your code as it stands now. In general it is better either to use booleans, True or False, or constants declared at the top of the code. Then you could have changed translated them easily, and not worry if you caught all the places it were used.

General game logic

Removing some post inflicted bugs, it is possible to play, but there are few issues related to general game logic and handling:

  • How do you flag mines? – Or your help section isn't very helpful. I had to read and search in the code to find that I could type a1f to flag the a1 cell.
  • How do you abort a game? - Given that the win/loose didn't work as expected I got stuck in a loop where I wasn't able to end the game, and had to resort to breaking the program flow. It would be good to have an option to quit at any time.
  • Recursive next game and end game logic – If I win or loose (in a working version), you trigger the replay() function, which in turns starts over again. This nests deeper, and deeper. It would be better to have a main loop at top level, so that when you call play() and the game ends, it returns to this level, and then you ask for replay().
  • Use the if __name__ == '__main__': construct – It's good that you've made a mainmenu() function, it would be even better if this was within the named construct, as this would allow for your script to be used as a module as well as being run from the command line. I.e. do:

    if __name__ == '__main__':
        mainmenu()
    

Some issues related to the highscore list

  • If you start of by displaying the highscore list it fails – I started testing to see the highscore list, and it failed as I don't have any high_scorelist.dat file yet... This by the way, yet another magic name, which should have been a constant at the top.
  • You hide time within the score() function – In my environment this breaks stuff. You can't use time as both an imported module and a local variable.
  • Every time to append to the highscores you remove one? – Will this not leave your highscorelist empty at all times? Looks kind of strange, but I haven't actually tested this part.

All in all, your code did work locally, but when posting it got a little scrambled and dysfunctional. You should clean up your code to avoid having magic words and number, and rather use constants at the start. And you should cleanup your naming and spacing to make the code easier to read and understand. Text handling and documenting (both internal to the code and for the user) is also areas you need to look into some more.

\$\endgroup\$
  • \$\begingroup\$ Thank you for the feedback, really helpfull! The whole code was written in Swedish at first but had to translate it to post it here. Unfortunately some Swedish words made it through... Thank once again! \$\endgroup\$ – hamp Dec 8 '15 at 7:58
2
\$\begingroup\$

I'm no expert, but here are several improvements I would make:

creating initial '0'-filled grid:

grid = [['0' for i in range(size)] for j in range(size)]

improving show_grid:

def show_grid(grid,size):
    horizontal = ' -'+size*'----'
    #writes the column numbers
    column = '   ' + '   '.join(string.ascii_uppercase[:size])
    print (column,'\n',horizontal)
    # writes row numbers
    for idx,i in enumerate(grid, start=1):
        row = str(idx)
        row += ' | ' + ' | '.join(i)
        print (row+'\n'+horizontal)

improving the surrounding_cells method:

def surrounding_cells(self,row,col,size):
    surroundings = []
    for i in range(max(row-1, 0), min(row+2, size)):
        for j in range(max(col-1, 0), min(col+2, size)):
            if i == row and j == col:
                continue
            surroundings.append((i,j))

Two notes on goodvalues(): if värde not in range(min,max): a = False is not needed, you can just remove it. And it's faster (and takes less memory) to check if min <= varde < max than if varde in range(min, max)

Control:

def control(grid, shown_grid, size, number_of_mines, start_time, username, flags, mines):
    empty_cells = sum(row.count('-') for row in shown_grid)
    cells_with_mines = sum(row.count('*') for row in shown_grid)
    if empty_cells == cells_with_mines or set(flags) == set(mines):
        result('YOU WON', grid, shown_grid, start_time, number_of_mines, size, username)

Also, naming needs a bit of fix: first of all, use snake_case instead of joiningallwordsinone, it would make your code much more readable. Also it is a good manner to use English (as you mostly do), and if you are not familiar with proper spelling, check with dictionary (or IDE built-in spellchecker). And last thing, even if for some reason you use non-English names, try at least to avoid non-ASCII symbols in them (like å, ø, etc.)

And I think, you might want to create a Game class, to store things like username, size of grid, number of mines, etc. So you can just take these values when you need them and you won't have to pass them around every time. (e.g. now it's pretty unclear why result() needs so many parameters).

\$\endgroup\$
  • \$\begingroup\$ And it's faster (and takes less memory) to check if min <= varde < max than if varde in range(min, max) <- benchmark needed \$\endgroup\$ – Caridorc Dec 7 '15 at 17:13
  • \$\begingroup\$ Thank you for amazing feedback! Got one question at the moment. def surrounding_cells(self,row,col,size): surroundings = [(i, j) for i in ] I wonder what is supposed to be after "in" :) Thank you once again! \$\endgroup\$ – hamp Dec 7 '15 at 17:14
  • \$\begingroup\$ @hamp, no, it shouldn't be there at all, I'll fix it. I was trying to use a generator, but it appears to be too long. \$\endgroup\$ – Alissa Dec 7 '15 at 17:43
  • \$\begingroup\$ @Caridorc, I don;t think a benchmark is needed to see that a double comparison is faster than creating an iterator and checking if it contains the value \$\endgroup\$ – Alissa Dec 7 '15 at 17:46
  • \$\begingroup\$ Benchmarking tells me it actually is faster than range, but on a million runs, it just was 0.7 seconds slower. So you were correct. But be careful about premature micro-optimization. \$\endgroup\$ – Caridorc Dec 7 '15 at 18:02

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.