I know this is a little bit late but I figured that code reviews never go out of style. Below I have rewritten this class to be slightly more idomatic Java - here are some of the highlights:
- Always prefer final variables, hence the set and list being changed to final
- Don't need to use the 'this' keyword as this is the implied scope. Though you should use it if it you have another variable in scope with the same name, as often is the case with setters. Because I edit Java in an IDE and the syntax colorer highlights fields different from other items I find 'this' to be too noisy.
- Camel Cased names that have a noun-verb arrangement. Typically java class methods are of the form verbNoun like setValue.
- Java does have a for-each looping construct so I made use of it in the list traversal in informGroups as it didn't appear that 'i' was used for anything other than list access.
- The only tricky thing I saw was in the inform_groups method which may have indeterminate behavior. Toda you are assuming that in this method that the list size is exactly one. In fact this is a necessary condition for this behavior of this method to be correct. Unfortunately, you call this method from two different places and will undoubtedly come back to this class one day and make a change that will call inform_groups when the possibles set size is > 1. Therefore I would add an assertion at the beginning of the method to document and verify this important precondition.
As a side note, in Java you are not guaranteed the order of the array that is produced to by toArray so the so the 0 index may be different each time you run this. There are ordered sets in Java like TreeSet which can give you better predictability here.
Overall this was a nice simple class and I liked it, your methods are nice and short, and so this class is fairly testable.
You may consider whether or not it is worth passing the possible values into the constructor so as to avoid hardcoding them if you decide to change the range.
Also, in the inform_groups method you are passing two pieces of state to the group, including the cell itself, which begs the question: why not just pass the cell and have a method called getSolvedValue which will return the solved value perhaps then you could get away with implementing a simpler interface?
Finally, it looks like the only use for the group is to call cellSolved and what you really have here is a listener/observer pattern. To avoid passing too much responsibility around in the form of a whole CellGroup, consider having the CellGroup implement an interface like CellSolveListener with exactly one method: cellSolved(Cell cell). And your registration method looks like registerSolveListener(CellSolvedListener listener). This documents clearly the contract between the two Objects, makes sure you are only passing around the responsibility you need and as a bonus Objects other than the CellGroup can now listen for changes.
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
public class Cell
{
private final Set<Integer> possibles = new HashSet<Integer>();
private final List<CellGroup> groups = new ArrayList<CellGroup>();
public Cell()
{
possibles.addAll( Arrays.asList( 1, 2, 3, 4, 5, 6, 7, 8, 9 ) );
}
public void registerCellGroup( CellGroup group )
{
groups.add( group );
}
public void setValue( Integer number )
{
possibles.clear();
possibles.add( number );
informGroups();
}
public void removeValue( Integer number )
{
possibles.remove( number );
if ( isSolved() )
{
informGroups();
}
}
public boolean isSolved()
{
return possibles.size() == 1;
}
private void informGroups()
{
assert possibles.size() == 1;
for ( CellGroup group : groups )
{
group.cellSolved( this, possibles.iterator().next());
}
}
}
static final
"constants" likeMAX_VALUE
) – Landei Mar 6 '12 at 9:04