Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm a ruby programmer who's writing Java for university. I'd love to get some feedback on how my Java looks and how to improve it to conform with the norms.

import java.util.Set;
import java.util.HashSet;
import java.util.List;
import java.util.ArrayList;

public class Cell {
  private Set<Integer> possibles;
  private List<CellGroup> groups;

  public Cell() {
    this.possibles = new HashSet<Integer>(java.util.Arrays.asList(1,2,3,4,5,6,7,8,9));
    this.groups = new ArrayList<CellGroup>();
  }

  public void register_cell_group(CellGroup group) {
    this.groups.add(group);
  }

  public void set(Integer number) {
    // Make the only possibile the set number
    this.possibles = new HashSet<Integer>();
    this.possibles.add(number);

    this.inform_groups();
  }

  public void not(Integer number) {
    this.possibles.remove(number);

    if (this.possibles.size() == 1) {
      this.inform_groups();
    }
  }

  public boolean is_solved() {
    return this.possibles.size() == 1;
  }

  private void inform_groups() {
    for (int i = 0; i < this.groups.size(); i++) {
      this.groups.get(i).cell_solved(this, this.possibles.toArray(new Integer[0])[0]);
    }
  }
} 
share|improve this question
2  
Additionally to Cygal's comments you should stick with Java naming conventions. Especially you should use camelCase for methods (underscores in names are only used in static final"constants" like MAX_VALUE) – Landei Mar 6 '12 at 9:04
Yeah, just getting my head around the new naming conventions. – thomasfedb Mar 6 '12 at 11:19

2 Answers

up vote 2 down vote accepted

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:

  1. Always prefer final variables, hence the set and list being changed to final
  2. 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.
  3. Camel Cased names that have a noun-verb arrangement. Typically java class methods are of the form verbNoun like setValue.
  4. 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.
  5. 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()); 
        }
    }
}
share|improve this answer
Thanks @adambender, excellent and very helpful. – thomasfedb Mar 27 '12 at 8:10
Glad I could be of assistance. Good luck learning java! – adambender Apr 11 '12 at 19:00

Edit: As Landei suggested, use camelCase! Thanks Landei.

  1. Your set function does not need to take an Integer. Idiomatic code would only use an int. Autoboxing means you can still add an int to this.possibles.
  2. Why don't you use is_solved in not?
  3. not is surprising, and I need to read the code to know what it does. I'd suggest naming it remove?
  4. What about this.possibles.iterator().next() instead of the toArray trick?
  5. Use a for each statement in inform_groups:

    for(CellGroup g : this.groups) {
         g.cell_solved(this, this.possibles.iterator().next());
    }
    
share|improve this answer
Thanks @Cygal, exactly what I was looking for. – thomasfedb Mar 6 '12 at 11:18

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.