6
\$\begingroup\$

I'm trying to write a Python class that creates a matrix of zeros, then uses a random number generator to pick 'seats' on the matrix. It changes the zero in that seat to a one, until the matrix is all ones. Can someone critique/correct my code? (I also want the 'agent' to check its surroundings on the matrix, and try 3 times to find a seat that is 2 seats away from any ones.)

import random
import numpy as np
#cs is grid size or seats, ns is number of agents
class Seats():
    def __init__(self, cs, ns):
        self.cs = cs
        self.ns = ns 
        self.seats = np.zeros([self.cs, self.cs],dtype=int)  


    def foundseat(self):
        foundseat = False         
        tries = 0
        while foundseat == False and tries <= 3:
            x = random.randint(0, self.cs)
            y = random.randint(0, self.cs)
            if self.seats[x][y] < 1:
                self.seats[x][y] = 1
                foundseat = True
            else: 
                tries += 1

   def goodseat(self, x,y):
    empty_neighbors = 0
    for neighbor_x in range(x-1,x+1):
        for neighbor_y in range(y-1,y+1):
            if self.seats[neighbor_x][neighbor_y] == 0:
                empty_neighbors = empty_neighbors + 1
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! Please edit your title to what your code do not what you want as a review! \$\endgroup\$ – Marc-Andre Apr 29 '16 at 12:29
  • 4
    \$\begingroup\$ What do you intend to use this algorithm for? Why does it give up after 3 tries? (Is that an intentional component of your algorithm, or a work-around to prevent lengthy looping when most seats are occupied?) \$\endgroup\$ – Anko Apr 29 '16 at 13:31
  • 1
    \$\begingroup\$ I'm also sort of confused as to the purpose of this algorithm. If the matrix is already known, there are much better techniques to find empty seats and even adjacent empty seats. Or is this a purely hypothetical program? \$\endgroup\$ – Barry Carter Apr 29 '16 at 18:57
3
\$\begingroup\$
  • Docstring: You should include a docstring at the beginning of every method/class/module you write. This will help documentation identify what your code is supposed to do.
  • Simplify boolean comparison: Instead of evaluating if found == False, you should evaluate the variable itself, if not found:, since it contains a boolean value.
  • Simplify variable addition: Instead of doing x = x + 1, you should do x += 1, since it's more compact and easier to read. You switch between these two in your program, but it good to stay consistent with one practice.
  • Variable Naming: You did a good job of naming the variables snake_case, but the method names are lacking. This is a link to PEP-8 Naming Conventions, which can provide more insight.

Updated Code

"""
Module Docstring:
A description about your program goes here
"""

import random
import numpy as np

class Seats():
    """
    A class for containing an array of seats, finding seats,
    and counting good seats
    """
    def __init__(self, cs, ns):
        """
        Seats Class Init Method

        :param cs: The size of the grid of seats
        :param ns: The number of agents

        """
        self.cs = cs
        self.ns = ns 
        self.seats = np.zeros([self.cs, self.cs], dtype=int)  

    def found_seat(self):
        """
        Attempts to find an open seat in the array `self.seats`
        """
        found = False
        tries = 0
        while not found and tries <= 3:
            x = random.randint(0, self.cs)
            y = random.randint(0, self.cs)
            if self.seats[x][y] < 1:
                self.seats[x][y] = 1
                found = True
            else:
                tries += 1

    def good_seat(self, x, y):
        """
        Determines the amount of empty neighbors around the seat

        :param x: The X coordinate of the seat
        :param y: The Y coordinate of the seat

        """
        empty_neighbors = 0
        for neighbor_x in range(x - 1, x + 1):
            for neighbor_y in range(y - 1, y + 1):
                if self.seats[neighbor_x][neighbor_y] == 0:
                    empty_neighbors += 1

\$\endgroup\$
1
\$\begingroup\$

Along with @Linny's changes which improve the code quality. You could start using more numpy specific stuff to do things like check neighbours for zeros.

  1. In numpy matrix, unlike in list of lists... you can use this format for indexing: seats[x][y] == seats[x, y].
  2. The two for loops can be turned into slicing and summing: (seats[neighbour_x-1:neighbour_x+1, neighbour_y-1:neighbour_y+1] == 0).sum(). In the brackets, it gets the range of matrix, then creates a bool matrix where it satisfies the condition and sums the count.
\$\endgroup\$

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.