Specifically, as the ruby-talk discussion title says, it’s the immutable classes (NilClass, FalseClass, TrueClass, Fixnum, and Symbol) that do this. Consider, for example, the following irb session:

irb 1:0> 5.respond_to? :dup
=> true
irb 2:0> 5.dup
TypeError: can't dup Fixnum
        from (irb):1:in `dup'
        from (irb):1
irb 3:0> 

If you don’t know Ruby, the first line asks the Fixnum object 5 if it responds to the method dup (with the name expressed as a symbol, hence the ”:”). The answer is true, becuase this method is defined by the module Kernel, which is included by the top-level class Object, an ancestor of Fixnum.

However, when you actually call dup on 5, it raises TypeError, as shown.

So, this looks like a classic Liskov Substitution Principle violation. The term for this code smell is Refused Bequest (e.g., see here) and it’s typically fixed with the refactoring Replace Inheritance with Delegation.

The email thread is about a proposal to change the library in one of several possible ways. One possibility is to remove dup from the immutable classes. This would eliminate the unexpected behavior in the example above, since 5.respond_to?(:dup) would return false, but it would still be an LSP violation, specifically it would still have the Refused Bequest smell.

One scenario where the current behavior causes problems is doing a deep copy of an arbitrary object graph. For immutable objects, you would normally just want dup to return the same object. It’s immutable, right? Well, not exactly, since you can re-open classes and even objects to add and remove methods in Ruby (there are some limitations for the immutables…). So, if you thought you actually duplicated something and started messing with its methods, you would be surprised to find the original was “also” modified.

So, how serious is this LSP issue (one of several)? When I pointed out the problem in the discussion, one respondent, Robert Dober, said the following (edited slightly):

I would say that LSP does not apply here simply because in Ruby we do not have that kind of contract. In order to apply LSP we need to say at a point we have an object of class Base, for example. (let the gods forgive me that I use Java)

void aMethod(final Base b){
   ....
}

and we expect this to work whenever we call aMethod with an object that is a Base. Anyway the compiler would not really allow otherwise.

SubClass sc;  // subclassing Base od course
aMethod( sc ); // this is expected to work (from the type POV).

Such things just do not exist in Ruby, I believe that Ruby has explained something to me:

• OO Languages are Class oriented languages
• Dynamic Languages are Object oriented languages.

Replace Class with Type and you see what I mean.

This is all very much IMHO of course but I feel that the Ruby community has made me evolve a lot away from “Class oriented”.

He’s wrong that the compiler protects you in Java; you can still throw exceptions, etc. The JDK Collection classes have Refused Bequests. Besides that, however, he makes some interesting points.

As a long-time Java programmer, I’m instinctively uncomfortable with LSP violations. Yet, the Ruby API is very nice to work with, so maybe a little LSP violation isn’t so bad?

As Robert says, we approach our designs differently in dynamic vs. static languages. In Ruby, you almost never use the is_a? and kind_of? methods to check for type. Instead, you follow the duck typing philosophy (“If it acts like a duck, it must be a duck”); you rely on respond_to? to decide if an object does what you want.

In the case of dup for the immutable classes, I would prefer that they not implement the method, rather than throw an exception. However, that would still violate LSP.

So, can we still satisfy LSP and also have rich base classes and modules?

There are many examples of traits that one object might or should support, but not another. (Those of you Java programmers might ask yourself why all objects support toString, for example. Why not also toXML...?)

Coming from an AOP background, I would rather see an architecture where dup is added only to those classes and modules that can support it. It shouldn’t be part of the standard “signature” of Kernel, but it should be present when code actually needs it.

In fact, Ruby makes this sort of AOP easy to implement. Maybe Kernel, Module, and Object should be refactored into smaller pieces and programmers should declaratively mixin the traits they need. Imagine something like the following:

irb 1:0> my_obj.respond_to? :dup
=> false
irb 2:0> include 'DupableTrait'  
irb 2:0> my_obj.respond_to? :dup
=> true
irb 4:0> def dup_if_possible items
irb 5:1>  items.map {|item| item.respond_to?(:dup) ? item.dup : item}
irb 6:1> end
...

In other words, Kernel no longer “exposes the dup abstraction”, by default, but the DupableTrait module “magically” adds dup to all the classes that can support it. This way, we preserve LSP, streamline the core classes and modules (SRP and ISP anyone?), yet we have the flexibility we need, on demand.

Today, the constructor looks something like this:

    def __init__( self, rows = 16, cols = 16, mines = 10): # 40 ):
        """Initialize the playing field.

        This function creates a playing field of the given size, and randomly
        places mines within it.

        rows and cols are the numbers of rows and columns of the playing  field, respectively.  
        mines is the number of mines to be placed within the field.
        """ 
        self._validate_parameters( rows, cols, mines )

        self.rows = rows
        self.cols = cols
        self.mines = mines
        self.cleared = 0
        self.flags = 0
        self.start_time = None

        minelist = []
        self.freecoords = {}
        for col in range( cols ):
            self.freecoords[col] = range( rows )

        self._place_mines( mines, minelist )

        self.board = []
        for col in range( cols ):
            self.board.append( [( -2, 0 )] * rows )
            for row in range( rows ):
                if ( row, col ) in minelist:
                    self.board[col][row] = ( -1, 0 )

    def _validate_parameters( self, rows, cols, mines ):
        for var in ( rows, cols, mines ):
            if var < 0:
                raise ValueError, "all arguments must be > 0" 
        if mines >= ( rows * cols ):
            raise ValueError, "mines must be < (rows * cols)" 

    def _place_mines( self, mines, minelist ):
        while mines > 0:
            y = random.choice( self.freecoords.keys() )
            x = random.randrange( len( self.freecoords[y] ) )
            minelist.append( ( self.freecoords[y][x], y ) )
            del self.freecoords[y][x]
            if not self.freecoords[y]:
                del self.freecoords[y]
            mines = mines - 1

I feel a little guilty for breaking out functions without testing them. I start to write tests for _validate_parameters, but I cannot call it in isolation. I have to have an instance of Field to call it, and it is also called in Field’s constructor. I can’t test weird values as easily as I’d like. I need something like a static method (c++/java). I add the @classmethod decorator and I can call it freely from my unit tests.

I remind the reader that the game runs and is playable, so the addition of tests should pretty much be a bit of pedantic formalism in preparation for refactoring. However, look at the code carefully and see if you can see a loophole:

    @classmethod
    def _validate_parameters( self, rows, cols, mines ):
        for var in ( rows, cols, mines ):
            if var < 0:
                raise ValueError, "all arguments must be > 0
        if mines >= ( rows * cols ):
            raise ValueError, "mines must be < (rows * cols)" 

A perceptive reader will realize that the code is checking that the value is less than zero, but reporting that it must be greater than zero. I found this with tests before it jumped out at me visually.

I write and run several tests and then get a nag from PyDev. I guess somehow I missed the part where PyDev told me it was not free software. The nag messages tells me I’m in a trial period and I’m expected to eventually buy or quit. Bummer. PyDev is not expensive. I just have to decide if I want to afford it.

I am also playing with Eric3 which is a python-specific IDE with refactoring, and it seems to have better integration with PyUnit. It is not as nice in terms of automatic completion and seems to have trouble with the context menu for refactoring. I immediately miss the cool way that PyDev would stick the word “self” in so I didn’t have to type it.

In the last installment, I extracted a method called _place_mines but it’s a bit hard to test. It’s actually a very poor refactoring, done to test a tool and not really to improve the code. As is, it wants to muck about with an unspoken parameter.

    def _place_mines( self, mines, minelist ):
        while mines > 0:
            y = random.choice( self.freecoords.keys() )
            x = random.randrange( len( self.freecoords[y] ) )
            minelist.append( ( self.freecoords[y][x], y ) )
            del self.freecoords[y][x]
            if not self.freecoords[y]:
                del self.freecoords[y]
            mines = mines - 1  

It’s small, but not obvious. Nor is it tested. It is using freecoords rather heavily. Breaking out that method was a mistake, and I put it back inline. It seems that freecords is a list of all the positions in the grid that do not contain mines. It is only used in the constructor and one other place. In the other place, it’s used and then deleted from the object.

The names of variables mines and minelist aren’t helpful because the names mean the same thing, even though the variables do not. They each mean more than one mine, but one is an integer count and the other is a list of the coordinates of the mines. That should change right away. I rename the integer to numberOfMines.

With the __init__ reconstituted, I notice that I don’t even keep the minelist. It’s discarded at the end of the __init__method. Maybe I can do this more simply.

I realize now that the code suffers from Tuple Madness. The cells are represented by two-tuples. The first and second item are not distinguished by a name, only by subscripts 0 and 1. There can hardly be anything less descriptive of the content. The initial values are (-2,0), but sometimes they are replaced with (-1,0). The code gives no obvious guidance to the significance of the first parameter versus the second parameter, and no real guide to the meaning of the integer-coded values. If one were going to document this code, it would be helpful to explain the use of anonymous data structures. Of course, that’s selfish because my goal is to obviate any comments I find. I just want it to be easy.

I dig in and discover that subscript zero really is the count of adjacent cells with mines. Since you can’t have less than zero mines, the number -2 tells us that we’ve not counted yet. The number -1 is a code telling us that the cell itself contains a mine. I’ll want to split these out later into named attributes.

Tuple Madness is such an easy trap to fall into with Python or Ruby (and I’m told Lisp as well). Each has such great native tuple and list types that we may forget to create real data structures for small data sets. I’ve done it, the PySweeper author fell into it, and so have my coworkers. If we had n-tuples in C, the resulting code would have been so inscrutible that the software practice might never have recovered. Yet two-tuples can be very good things. Just not here. I vow to create a data structure for the cell as soon as I have this body of code sufficiently under test.

As I learn about the two-tuples I write a couple of tests, as if there were more descriptive functions. I write calls to such functions as hasAMine() to check if subscript zero has a -1 value, and cellAt to get a cell at a given location, etc. Explanatory functions are a powerful documentation technique

    def cellAt(self, x, y):
        return self.board[x][y]

    def isOpened( self, x, y ):
        return self.cellAt(x, y)[1] == -1

    def countAt(self, x,y):
        result = self.cellAt(x,y)[0]
        if result < 0:  result = None  # indicator value
        return result

    def hasMine(self, x,y):
        return self.cellAt(x,y)[0] == -1
  

I have a small-but-growing body of tests. I should soon have enough confidence to do bigger refactorings.

Note the ugly comment in countAt. This is variable reuse. Rather than have a separate indicator (another element in the tuple maybe), one element is serving double-duty. It is too entrenched right now, so I’ll work on making it obvious and easier to separate later. Soon my body of explanatory methods will compose an opaque interface, and there will be little or no upset when I change the implementation of those methods.

Note also that we’re in violation of the SRP. The class has methods related to the field and also to specific cells in the board. Maybe I will eventually have classes for the cells, the game, and the board.

Time to stop reading and do more testing. I did learn enough to know that the game tries to be “fair”. If there is a mine on the very first square you select, it moves it out of the way. My “gaming the system” gene took over. I should be able to fill a three-by-three grid with eight mines, and then click (open) the center square. That should guarantee me that I have this structure:

mine	mine	mine
mine	empty	mine
mine	mine	mine

The more astute reader knows that this disregards the first rule of testing: Control The Environment. I should take control of the placement of mines rather than testing the system with random mine placement.

Sure enough, I find that the routine that moves the mine to a new spot is not working quite as expected. Now I have a failing test which allows me to make changes to the randomizer. I fix it, and then decide to pull the random chooser into a separate class. It will behave according to the iterator protocol in Python so I can pass a simple array of (col,row] pairs to the constructor as a testing stub/mock/thing.

Now the top of my file has the random cell chooser. There are some named constants, the still-busy __init__ method, the now-tested _validate_parameters method, and then other code we’ve either talked about already or haven’t touched yet.

random.seed()

class random_cell_chooser(object):
    def __init__(self, columns, rows):
        self.choices = [ (x,y) for x in range(columns) for y in range(rows) ]
    def next(self):
        if len(self.choices) == 0:
            raise StopIteration, "Out of free cells" 
        item = random.choice(self.choices)
        self.choices.remove(item)
        return item

INITIALLY_EMPTY = ( -2, 0 )
INITIALLY_MINED = ( -1, 0 )

class Field:
    """Provide a playing field for a Minesweeper game.

    This class internally represents a Minesweeper playing field, and provides
    all functions necessary for the basic manipulations used in the game.
    """ 
    def __init__( self, rows = 16, columns = 16, numberOfMines = 40, chooser=None):
        """Initialize the playing field.
        """ 

        self._validate_parameters( rows, columns, numberOfMines )

        self.rows = rows
        self.cols = columns
        self.mines = numberOfMines
        self.selector = chooser or random_cell_chooser(columns,rows)
        self.cleared = 0
        self.flags = 0
        self.start_time = None

        self.board = []
        for col in range( columns ):
            self.board.append( [INITIALLY_EMPTY] * rows )

        for counter in range(numberOfMines):
            (col,row) = self.selector.next()
            self.board[col][row] = INITIALLY_MINED

    @classmethod
    def _validate_parameters( self, rows, cols, mines ):
        for var in ( rows, cols, mines ):
            if var < 0:
                raise ValueError, "all arguments must be > 0" 
        if mines >= ( rows * cols ):
            raise ValueError, "mines must be < (rows * cols)" 

  

An engineer friend of mine is fond of saying “suspenders and belt”, referring to having redundant safety mechanisms. I’m appreciating the value of both a good version control system and a growing set of tests. I am aware of the quality of the code I’m modifying (whether it passes all the tests) and that I can always abandon a change I’m making if it gets messy.

My little test is building that three-by-three grid and playing a very simple game to completion by opening the middle square. I am testing setup and a bit of gameplay. I’ll put a better test in tomorrow.

It is a simple thing to extract the method for placing the mines and pass it to the constructor for Field. Now I can set the mines just where I want them, giving me total control of the field constructor. I make it act like an iterator so my tests can pass a list iterator to the constructor:

class random_cell_chooser(object):
    def __init__(self, columns, rows):
        self.choices = [ (x,y) for x in range(columns) for y in range(rows) ]
    def next(self):
        if len(self.choices) == 0:
            raise StopIteration, "Out of free cells" 
        item = random.choice(self.choices)
        self.choices.remove(item)
        return item

  

Because the tests are new and few, I’m also playing (losing) a game of minesweeper every so often so that I know that it still beats me in reasonable ways. I can’t wait until I can depend entirely on automated tests. Maybe in this way, it would have been better to have some basic acceptance tests before working in unit tests. It’s a thought.

Here’s what it is looks like right now.

import os
import unittest
import sys

def collect_tests(directoryName):
    """Gather all tests matching "test*.py" in the current directory""" 
    alltests = []
    for filename in os.listdir(directoryName):
        if filename.startswith('test') and filename.endswith('.py'):
            module = __import__(filename.replace(".py",""))
            for item_name in dir(module):
                item = getattr(module,item_name)
                if isinstance(item,type) and issubclass(item,unittest.TestCase):
                    test = unittest.makeSuite(item)
                    alltests.append(test)
    return alltests

def suite():
    "Collect all local tests into a monster suite" 
    suite = unittest.TestSuite()
    here = os.path.dirname(sys.argv[0]) or os.getcwd()
    alltests = collect_tests(here)
    suite.addTests(alltests)
    return suite

if __name__ == "__main__":
    tester = unittest.TextTestRunner()
    tester.run(suite())

Thre may be easier ways or better ways, but right now I don’t know them. I figure it will be good enough until I learn something cooler. I can think of several ways to extend it, but don’t need them.