3
\$\begingroup\$

I have made GPL software in GitHub whose purpose is reading shogi notations (shogi is Japanese chess). I have been told that my software modelling is underdeveloped in this question and advised to post a question about the classes I made to manage the board, so here I go:

The main concern is about the file managers.py, which contains the classes that I call "managers". Two of those managers are for handling the board's coords for pieces.

One of them is the class coords_manager - managers.py line 27:

class coords_manager:
    def __init__(self):
        # Pieces arrays
        self.lista_pn = None
        self.lista_spn = None
        self.cnt_pn = None
        self.rpn = None
        self.lista_pb = None
        self.lista_spb = None
        self.cnt_pb = None
        self.rpb = None
        self.lista_ln = None
        self.lista_sln = None
        self.cnt_ln = None
        self.rln = None
        self.lista_lb = None
        self.lista_slb = None
        self.cnt_lb = None
        self.rlb = None
        self.lista_nn = None
        self.lista_snn = None
        self.cnt_nn = None
        self.rnn = None
        self.lista_nb = None
        self.lista_snb = None
        self.cnt_nb = None
        self.rnb = None
        self.lista_sn = None
        self.lista_ssn = None
        self.cnt_sn = None
        self.rsn = None
        self.lista_sb = None
        self.lista_ssb = None
        self.cnt_sb = None
        self.rsb = None
        self.lista_gn = None
        self.cnt_gn = None
        self.rgn = None
        self.lista_gb = None
        self.cnt_gb = None
        self.rgb = None
        self.lista_tn = None
        self.lista_stn = None
        self.cnt_tn = None
        self.rtn = None
        self.lista_tb = None
        self.lista_stb = None
        self.cnt_tb = None
        self.rtb = None
        self.lista_bn = None
        self.lista_sbn = None
        self.cnt_bn = None
        self.rbn = None
        self.lista_bb = None
        self.lista_sbb = None
        self.cnt_bb = None
        self.rbb = None
        self.rey_n = None
        self.rey_b = None

        self.reverted = 1 #1 OR -1
        self.coords_ax = {
            '1': 837,
            '2': 766,
            '3': 695,
            '4': 624,
            '5': 553,
            '6': 482,
            '7': 411,
            '8': 340,
            '9': 269
        }
        self.coords_ay = {
            'i': 589,
            'h': 518,
            'g': 447,
            'f': 376,
            'e': 305,
            'd': 234,
            'c': 163,
            'b': 92,
            'a': 21
        }
        self.coords_bx = {
            '1': 269,
            '2': 340,
            '3': 411,
            '4': 482,
            '5': 553,
            '6': 624,
            '7': 695,
            '8': 766,
            '9': 837
        }
        self.coords_by = {
            'a': 589,
            'b': 518,
            'c': 447,
            'd': 376,
            'e': 305,
            'f': 234,
            'g': 163,
            'h': 92,
            'i': 21
        }
        self.coords_x = None
        self.coords_y = None
        self.update()
        self.begin()

    def begin(self):
        self.lista_pn = {1:[self.coords_x['1'],self.coords_y['g']],2:[self.coords_x['2'],self.coords_y['g']],3:[self.coords_x['3'],self.coords_y['g']],4:[self.coords_x['4'],self.coords_y['g']],5:[self.coords_x['5'],self.coords_y['g']],6:[self.coords_x['6'],self.coords_y['g']],7:[self.coords_x['7'],self.coords_y['g']],8:[self.coords_x['8'],self.coords_y['g']],9:[self.coords_x['9'],self.coords_y['g']]}
        self.lista_spn = {}
        self.cnt_pn = 10
        self.rpn = 0
        self.lista_pb = {1:[self.coords_x['1'],self.coords_y['c']],2:[self.coords_x['2'],self.coords_y['c']],3:[self.coords_x['3'],self.coords_y['c']],4:[self.coords_x['4'],self.coords_y['c']],5:[self.coords_x['5'],self.coords_y['c']],6:[self.coords_x['6'],self.coords_y['c']],7:[self.coords_x['7'],self.coords_y['c']],8:[self.coords_x['8'],self.coords_y['c']],9:[self.coords_x['9'],self.coords_y['c']]}
        self.lista_spb = {}
        self.cnt_pb = 10
        self.rpb = 0
        self.lista_ln = {1:[self.coords_x['1'],self.coords_y['i']],2:[self.coords_x['9'],self.coords_y['i']]}
        self.lista_sln = {}
        self.cnt_ln = 3
        self.rln = 0
        self.lista_lb = {1:[self.coords_x['1'],self.coords_y['a']],2:[self.coords_x['9'],self.coords_y['a']]}
        self.lista_slb = {}
        self.cnt_lb = 3
        self.rlb = 0
        self.lista_nn = {1:[self.coords_x['2'],self.coords_y['i']],2:[self.coords_x['8'],self.coords_y['i']]}
        self.lista_snn = {}
        self.cnt_nn = 3
        self.rnn = 0
        self.lista_nb = {1:[self.coords_x['2'],self.coords_y['a']],2:[self.coords_x['8'],self.coords_y['a']]}
        self.lista_snb = {}
        self.cnt_nb = 3
        self.rnb = 0
        self.lista_sn = {1:[self.coords_x['3'],self.coords_y['i']],2:[self.coords_x['7'],self.coords_y['i']]}
        self.lista_ssn = {}
        self.cnt_sn = 3
        self.rsn = 0
        self.lista_sb = {1:[self.coords_x['3'],self.coords_y['a']],2:[self.coords_x['7'],self.coords_y['a']]}
        self.lista_ssb = {}
        self.cnt_sb = 3
        self.rsb = 0
        self.lista_gn = {1:[self.coords_x['4'],self.coords_y['i']],2:[self.coords_x['6'],self.coords_y['i']]}
        self.cnt_gn = 3
        self.rgn = 0
        self.lista_gb = {1:[self.coords_x['4'],self.coords_y['a']],2:[self.coords_x['6'],self.coords_y['a']]}
        self.cnt_gb = 3
        self.rgb = 0
        self.lista_tn = {1:[self.coords_x['2'],self.coords_y['h']]}
        self.lista_stn = {}
        self.cnt_tn = 2
        self.rtn = 0
        self.lista_tb = {1:[self.coords_x['8'],self.coords_y['b']]}
        self.lista_stb = {}
        self.cnt_tb = 2
        self.rtb = 0
        self.lista_bn = {1:[self.coords_x['8'],self.coords_y['h']]}
        self.lista_sbn = {}
        self.cnt_bn = 2
        self.rbn = 0
        self.lista_bb = {1:[self.coords_x['2'],self.coords_y['b']]}
        self.lista_sbb = {}
        self.cnt_bb = 2
        self.rbb = 0
        self.rey_n = [self.coords_x['5'],self.coords_y['i']]
        self.rey_b = [self.coords_x['5'],self.coords_y['a']]

    def update(self):
        if self.reverted == 1:
            self.coords_x = self.coords_ax
            self.coords_y = self.coords_ay
        else:
            self.coords_x = self.coords_bx
            self.coords_y = self.coords_by

    def revert(self):
        self.reverted *= -1

        revert_x = {
            269: 837,
            340: 766,
            411: 695,
            482: 624,
            553: 553,
            624: 482,
            695: 411,
            766: 340,
            837: 269
        }
        revert_y = {
            21: 589,
            92: 518,
            163: 447,
            234: 376,
            305: 305,
            376: 234,
            447: 163,
            518: 92,
            589: 21
        }
        for k, e in self.lista_pn.items():
            self.lista_pn[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_spn.items():
            self.lista_spn[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_pb.items():
            self.lista_pb[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_spb.items():
            self.lista_spb[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_ln.items():
            self.lista_ln[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_sln.items():
            self.lista_sln[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_lb.items():
            self.lista_lb[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_slb.items():
            self.lista_slb[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_nn.items():
            self.lista_nn[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_snn.items():
            self.lista_snn[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_nb.items():
            self.lista_nb[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_snb.items():
            self.lista_snb[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_sn.items():
            self.lista_sn[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_ssn.items():
            self.lista_ssn[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_sb.items():
            self.lista_sb[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_ssb.items():
            self.lista_ssb[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_gn.items():
            self.lista_gn[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_gb.items():
            self.lista_gb[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_tn.items():
            self.lista_tn[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_stn.items():
            self.lista_stn[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_tb.items():
            self.lista_tb[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_stb.items():
            self.lista_stb[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_bn.items():
            self.lista_bn[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_sbn.items():
            self.lista_sbn[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_bb.items():
            self.lista_bb[k] = [revert_x[e[0]],revert_y[e[1]]]
        for k, e in self.lista_sbb.items():
            self.lista_sbb[k] = [revert_x[e[0]],revert_y[e[1]]]
        self.rey_n = [revert_x[self.rey_n[0]],revert_y[self.rey_n[1]]]
        self.rey_b = [revert_x[self.rey_b[0]],revert_y[self.rey_b[1]]]

        self.update()

The other one is matrix_manager - managers.py line 436:

class matrix_manager:
    def __init__(self):
        self.ADAPTX = {'9':0,'8':1,'7':2,'6':3,'5':4,'4':5,'3':6,'2':7,'1':8}
        self.ADAPTY = {'a':0,'b':1,'c':2,'d':3,'e':4,'f':5,'g':6,'h':7,'i':8}
        self.coords_hx = {
            837:'1',
            766:'2',
            695:'3',
            624:'4',
            553:'5',
            482:'6',
            411:'7',
            340:'8',
            269:'9'
        }
        self.coords_hy = {
            589:'i',
            518:'h',
            447:'g',
            376:'f',
            305:'e',
            234:'d',
            163:'c',
            92:'b',
            21:'a'
        }
        # X es el 1er índice
        self.matrix = [
                [1,1,1,1,1,1,1,1,1],
                [0,1,0,0,0,0,0,1,0],
                [1,1,1,1,1,1,1,1,1],
                [0,0,0,0,0,0,0,0,0],
                [0,0,0,0,0,0,0,0,0],
                [0,0,0,0,0,0,0,0,0],
                [1,1,1,1,1,1,1,1,1],
                [0,1,0,0,0,0,0,1,0],
                [1,1,1,1,1,1,1,1,1]
        ]
    def empty(self, coords_h):
        self.matrix[self.ADAPTY[coords_h[1]]][self.ADAPTX[coords_h[0]]] = False

    def fill(self, coords_h):
        self.matrix[self.ADAPTY[coords_h[1]]][self.ADAPTX[coords_h[0]]] = True

    def get_hcoords(self, coords):
        return str(self.coords_hx[coords[0]])+str(self.coords_hy[coords[1]])

    def check_ln(self, h_begin, h_destiny):
        cursorx = self.ADAPTX[h_begin[0]]
        cursory = self.ADAPTY[h_begin[1]]
        while cursory != self.ADAPTY[h_destiny[1]]+1:
            cursory -= 1
            if self.matrix[cursory][cursorx] == True:
                return False
        return True

    def check_lb(self, h_begin, h_destiny):
        cursorx = self.ADAPTX[h_begin[0]]
        cursory = self.ADAPTY[h_begin[1]]
        while cursory != self.ADAPTY[h_destiny[1]]-1:
            cursory += 1
            if self.matrix[cursory][cursorx] == True:
                return False
        return True

    def check_t(self, h_begin, h_destiny):
        cursorx = self.ADAPTX[h_begin[0]]
        cursory = self.ADAPTY[h_begin[1]]
        destx = self.ADAPTX[h_destiny[0]]
        desty = self.ADAPTY[h_destiny[1]]

        if cursorx == destx:
            mod = 1 if cursory < desty else -1
            while cursory != desty - mod:
                cursory += mod
                if self.matrix[cursory][cursorx] == True:
                    return False
            return True
        else:
            mod = 1 if cursorx < destx else -1
            while cursorx != destx - mod:
                cursorx += mod
                if self.matrix[cursory][cursorx] == True:
                    return False
            return True

    def check_b(self, h_begin, h_destiny):
        cursorx = self.ADAPTX[h_begin[0]]
        cursory = self.ADAPTY[h_begin[1]]
        destx = self.ADAPTX[h_destiny[0]]
        desty = self.ADAPTY[h_destiny[1]]

        modx = 1 if cursorx < destx else -1
        mody = 1 if cursory < desty else -1

        while (cursorx != destx - modx) or (cursory != desty - mody):
            cursorx += modx
            cursory += mody
            if self.matrix[cursory][cursorx] == True:
                return False
        return True

coords_manager handles dictionaries of coords of pieces that are present on the board. It also handles revertion of board. There is a list for each kind of piece (promoted pieces count as kind of piece for this) for each player. Doing it like this (instead of a global list for all pieces with a parameter to determine the kind of piece) allows me to make loops to iterate only over certain kind of piece. Another secondary function of coords_manager is to provide dictionaries to translate human-readable coords in numeric coords to blit the sprites. Every list of pieces gets the coords in numeric version.

Kinds of pieces are abbreviated this way:

s -> if present, it would mean promoted piece
p -> english abbreviature for the kind of piece - pawn in this case
n -> player's color (n if 'black' or 'sente', b if 'white' or 'gote')

When I noticed that I needed a way to prevent pieces jumping over other pieces, I created matrix_manager to resolve the problem. It basically handles a matrix of boolean values to keep in account which squares are filled by a piece.

The point I'm trying to make is that I want to make a Move class to clean up the code to prevent abuse of exec statements and regexp matching, as pointed in my previous question. But I'm convinced that, before doing such a thing, I should improve the managers first, as @200_success♦ suggested me in the previous question.

Which ways would you try to improve the board managers? Should I join those classes into a single one for a better modelling? Which deficiencies or redundancies am I not noticing in these managers?

I started this project at first just because I wanted a way to read my shogi notations. When the basic functionality worked, I decided to make the code public under GPL license for sharing, and with the hope that some entusiasts would join me to suggest improvements or even to work on the code if they felt willing to. So, you will find extensive general information in the README section in GitHub. Any little contribution is highly appreciated.

\$\endgroup\$
  • 3
    \$\begingroup\$ Step 1: you see how you've got almost the same code over and over again? That's an immediate sanction from the Redundancy Department of Redundancy. You clearly know how to use lists and dictionaries, so use them to reduce duplication. DRY! \$\endgroup\$ – jonrsharpe Jul 12 '15 at 22:37
  • \$\begingroup\$ Your comment made me laugh, but you are completely right. I've started inmediately to search about DRY principles on Google, it seems I don't know them well enough. Good point. Until you lectured me, I didn't know repetition could be avoided so far (assuming you are referencing the loops over dictionaries and similar patterns-> yeah, they are ridiculously redundant! :-$) Maybe exec statements would be useful for that (or I would end up abusing on them again)... 'Use lists/dictionaries to avoid repetition'... I have to think deeply about that... \$\endgroup\$ – SebasSBM Jul 12 '15 at 22:49
  • \$\begingroup\$ Gotcha! The first bite to the elephant: It won't reduce the amount of lines, but it can help sparing CPU cycles; I can use lists or dictionaries for that, thus avoiding redundant calculations on coords_manager.begin(), maybe placing them in global constants (or better alternative, due to it's not good abusing on globals too). I could start from here... another day... gotta sleep... work tomorrow... ZzZz... \$\endgroup\$ – SebasSBM Jul 12 '15 at 23:12
2
\$\begingroup\$

A few pointers:

  1. Python has a style guide, which you should strongly consider following (some docstrings would be nice, too);
  2. Rather than using an integer to determine when you can and can't revert, why not a boolean (True/False)? Then the test in update becomes if self.reverted, and in revert you can have self.reverted = not self.reverted;
  3. The dictionary literals that never change (e.g. revert_x) don't need to be rebuilt every time you call a method - make them class attributes, and give them UPPERCASE_WITH_UNDERSCORES names to indicate that they're constants; and
  4. Rather than the endless copies of the same thing over and over again, use data structures to abstract the issue (e.g. rather than self.lista_lb, why not use nested dictionaries to access self.list['a']['lb']?) - this will allow you to significantly reduce duplication by looping over the structures.
\$\endgroup\$
  • \$\begingroup\$ Awesome pointers. I've read the entire styleguide, I broke the conventions more than I imagined. My code probably looks too messy because of that. That said, my main priority now must be cleaning up all the broken conventions before I start altering the structure itself, for readability's sake. \$\endgroup\$ – SebasSBM Jul 13 '15 at 18:04
  • \$\begingroup\$ It took me 3 hours, but I've clean up the main script. Another day I'll do the same with the others. I'd like to give you credit in my commit's description if you don't mind... \$\endgroup\$ – SebasSBM Jul 13 '15 at 22:59
  • \$\begingroup\$ @SebasSBM I don't mind at all; you can credit me here or via my GitHub account (github.com/textbook) \$\endgroup\$ – jonrsharpe Jul 14 '15 at 6:01
  • \$\begingroup\$ @jonsharpe I have cleaned somemore my code. Now all the mainscripts follow PEP8, also I did some structural changes applying your 4th pointer, which reduce a little of duplication and prepare the way to somemore optimization. Here they are -> (github.com/SebasSBM/shogi_reader/tree/…) \$\endgroup\$ – SebasSBM Oct 25 '15 at 19:11
  • \$\begingroup\$ @jonsharpe About point 2: The good reason I used an Integer instead of a Boolean for revertion, is because I use this revertion value (1 or -1) as a multiplier for coords_sum (so it influences the piece's movement depending on revertion). This is done in function move_forward() -> see here in line 96 github.com/SebasSBM/shogi_reader/blob/… \$\endgroup\$ – SebasSBM Oct 25 '15 at 19:31

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.