Refactoring Exercise

[Junilu Lacar]

Since there haven't been very many discussions about refactoring in this forum, I have taken the liberty of copying some code from the JiG Beginner forum to use as an exercise.

<font size=+0>
/**
 * Simulate a game of Rock, Paper, Scissors
 */
public class Game 
{
    public static void main(String args[])
    {
        Player p1 = new Player();
        Player p2 = new Player();
        boolean gameWon = false;  
        int roundsPlayed = 0;    // Number of rounds played
        int p1Wins = p1.wins;
        int p2Wins = p2.wins;
        int draw = 0;
        String p1Choice;
        String p2Choice;
        // Game Loop
        do 
        {
            System.out.println("***** Round: " +
                roundsPlayed + " *********************\n");
            System.out.println("Number of Draws: "+ 
                draw + "\n");
            p1Choice = p1.playerChoice();
            System.out.println("Player 1: " + p1Choice + 
                "\t Player 1 Total Wins: " + p1Wins);
            p2Choice = p2.playerChoice();
            System.out.println("Player 2: " + p2Choice+ 
                "\t Player 2 Total Wins: " + p2Wins);
            if((p1Choice.equals("rock"))&&(p2Choice.equals("paper")))
            {
                System.out.println("Player 2 Wins");
                p2Wins++;  // trying a couple different ways to get count to work
            }
            else if((p1Choice.equals("paper"))&&(p2Choice.equals("rock")))
            {
                p1Wins++;
                System.out.println("Player 1 Wins");
            }
            else if((p1Choice.equals("rock"))&&(p2Choice.equals("scissors")))
            {
                p1Wins = p1.setWins();
                System.out.println("Player 1 Wins");
            }
            else if((p1Choice.equals("scissors"))&&(p2Choice.equals("rock")))
            {
                p2Wins = p2.setWins();
                System.out.println("Player 2 Wins");
            }
            else if((p1Choice.equals("scissors"))&&(p2Choice.equals("paper")))
            {
                p1Wins = p1.setWins();
                System.out.println("Player 1 Wins");
            }
            else if((p1Choice.equals("paper"))&&(p2Choice.equals("scissors")))
            {
                p2Wins = p2.setWins();
                System.out.println("Player 2 Wins");
            }
            if(p1Choice==p2Choice)
            {
                draw++;
                System.out.println("\n\t\t\t Draw \n");
            }
            roundsPlayed++;
            if((p1.getWins()>=3)| |(p2.getWins()>=3))
            {
                gameWon = true;
                System.out.println("GAME WON");
            }
            System.out.println();
        } while(gameWon != true);
    }
}
/**
 *
 */
class Player{
    int wins;      // # of wins
    int winTotal;
    /**
     * Randomly choose rock, paper, or scissors
     */
    public String playerChoice()
    {
        String choice = "";
        int c = (int)(Math.random()*3);
        switch(c) {
            case 0:
                choice = ("rock");
                break;
            case 1:
                choice = ("paper");
                break;
            case 2:
                choice = ("scissors");
                break;
        }
        return choice;
    }
    public int setWins() 
    {
        int winTotal = wins++;
        return winTotal;
    }
    public int getWins() 
    {
        return(wins);
    }
}

Would anybody like to start the refactoring?

[This message has been edited by JUNILU LACAR (edited June 16, 2001).]

[Frank Carver]

Do you want this to be strictly refactoring, or can we (for example) make small functionality changes to make it easier to unit test ?

[Junilu Lacar]

Well, that's one of the points I hoped someone would bring up right away
One very important piece of refactoring are the unit tests. Before making any changes to the code/design, we need to have unit tests so that we can make changes confidently.
Another point I hoped somebody would bring up was this: Wouldn't it be easier to just start over and rewrite this?
discussion please...

[Daniel Dunleavy]

You can check for a choice = for draw first. Then ifs for each player 1 win, and then use else player 2 wins. It would reduce the ifs.
Dan
[This message has been edited by Daniel Dunleavy (edited June 15, 2001).]

[Junilu Lacar]

Well, my first for refactoring would be to encapsulate the game rules somewhere. Using a test-first approach, I would have formulated the following test to be used with the JUnit unit test framework:
<pre>
public void testGameRules() {
assertEquals(true, Choice.ROCK.beats(Choice.SCISSORS));
assertEquals(false, Choice.ROCK.beats(Choice.ROCK));
assertEquals(false, Choice.ROCK.beats(Choice.PAPER));
assertEquals(true, Choice.SCISSORS.beats(Choice.PAPER));
assertEquals(false, Choice.SCISSORS.beats(Choice.ROCK));
assertEquals(false, Choice.SCISSORS.beats(Choice.SCISSORS));
assertEquals(true, Choice.PAPER.beats(Choice.ROCK));
assertEquals(false, Choice.PAPER.beats(Choice.SCISSORS));
assertEquals(false, Choice.PAPER.beats(Choice.PAPER));
}
</pre>
This hints at a Choice class that uses the typesafe enum pattern. Anybody care to take a stab at writing that class?

[Wirianto Djunaidi]

Here's my first attempt:
<pre>
public class Choice
{
private String name;
private Choice() { };
private Choice(String name)
{
this.name = name;
}
public String getName()
{
return name;
}
public static final Choice ROCK = new Choice("Rock")
{
public boolean beats(Choice target)
{
return target.getName().equals("Scissors") ? true : false;
}
};

public static final Choice SCISSORS = new Choice("Scissors")
{
public boolean beats(Choice target)
{
return target.getName().equals("Paper") ? true : false;
}
};

public static final Choice PAPER = new Choice("Paper")
{
public boolean beats(Choice target)
{
return target.getName().equals("Rock") ? true : false;
}
};
}
</pre>

[Junilu Lacar]

Thanks Ryo. Here are a few comments:
Try to follow the Pragmatic Programmer's DRY principle: Don't Repeat Yourself. There are a few places where you basically repeat the code: code for beat() and you have each of the literal strings "Rock", "Scissors", "Paper" twice. If for any reason you decide to change the descriptions, you'll need to change 6 rather than 3 strings.
I have to admit, your use of anonymous inner classes to encapsulate each rule is really interesting. In contrast, consider the version below. It doesn't repeat any of the string constants. Also, it lumps all the rules together. I'm not saying this is a better approach though. I'd like to hear some comments for or against one or the other.
<pre>
public class Choice {
public static final Choice ROCK = new Choice("Rock");
public static final Choice PAPER = new Choice("Paper");
public static final Choice SCISSORS = new Choice("Scissors");

private final String name;

private Choice(String name)
{
this.name = name;
}

public boolean beats(Choice opponent)
{
return ((this == ROCK) && (opponent == SCISSORS)) | |
((this == SCISSORS) && (opponent == PAPER)) | |
((this == PAPER) && (opponent == ROCK));
}

public String toString()
{
return this.name;
}
}
</pre>
(straining) "Can't..keep..vertical bars.. for..OR..together...

[This message has been edited by JUNILU LACAR (edited June 15, 2001).]

[Junilu Lacar]

I suppose you could have
<pre>
public abstract class Choice
{
public static final Choice ROCK;
public static final Choice SCISSORS;
public static final Choice PAPER;

static
{
ROCK = new Choice("Rock") {
public boolean beats(Choice opponent)
{
return (opponent == SCISSORS);
}
};

SCISSORS = new Choice("Scissors") {
public boolean beats(Choice opponent)
{
return (opponent == PAPER);
}
};

PAPER = new Choice("Paper") {
public boolean beats(Choice opponent)
{
return (opponent == ROCK);
}
};
} // end static init block

public abstract boolean beats(Choice opponent);

private final String name;

private Choice(String name)
{
this.name = name;
}
}
</pre>
Maybe that would be even better...
Comments, anyone?
formatting code here is such a PITA
Doh! made the Choice class abstract and added abstract beats() method so that it actually compiles
[This message has been edited by JUNILU LACAR (edited June 15, 2001).]
[This message has been edited by JUNILU LACAR (edited June 15, 2001).]

[Junilu Lacar]

One last post before I go away for the weekend:
I'd like to see main() pared down to something like:
<pre>
Game game = new Game(...);

while ( ! game.isOver() )
{
game.playRound();
game.showScore();
}
game.showResults();
</pre>
Can somebody try to do that while I'm away?

[Desai Sandeep]

Hi,
I have designed the above problem using Polymorphism and Singleton Pattern.
Reasons:

• Since there are EXACTLY 3 choices make an abstract superclass and subclass it to implement the three choices - Rock, Scissors and Paper, on the basis of Polymorphism.
  
• Since we donot require multiple instances of the subclass we can design the subclasses using Singleton pattern
  



<pre><font size=+0>
public abstract class Choice {
    private String name;
   
    protected Choice(String name) {
        this.name = name; 
    }     
    protected String getName() {
        return name; 
    }
    
    public static Choice getChoice(String name) {
        if (name.equals("Rock")) {
            return Rock.getRockChoice();  
        } else if (name.equals("Scissors")) {
            return Scissors.getScissorsChoice();  
        } else {
            return Paper.getPaperChoice();
        }
    }
    public abstract boolean beats(Choice choice); 
}
class Rock extends Choice {
    private static Rock rock; 
    private Rock() {
        super("Rock");
    }
    protected static Choice getRockChoice() {
        if (rock == null) {
           rock = new Rock();
        }   
        return rock;
    }      
 
    public boolean beats(Choice choice) {
        return choice.getName().equals("Scissors") ? true : false;
    } 
}

class Scissors extends Choice {
    private static Scissors scissors; 

    private Scissors() {
        super("Scissors");
    }     
    protected static Choice getScissorsChoice() {
        if (scissors == null) {
           scissors = new Scissors();
        }   
        return scissors;
    }      
 
    public boolean beats(Choice choice) {
        return choice.getName().equals("Paper") ? true : false;
    } 
}
class Paper extends Choice {
    private static Paper paper; 
    private Paper() {
        super("Paper");
    }     
    protected static Choice getPaperChoice() {
        if (paper == null) {
           paper = new Paper();
        }   
        return paper;
    }      
 
    public boolean beats(Choice choice) {
        return choice.getName().equals("Rock") ? true : false;
    } 
}
</pre></font>


Comments, please!May be my code requires a bit a refactoring
- Sandeep
[This message has been edited by Desai Sandeep (edited June 17, 2001).]

[Junilu Lacar]

Sandeep,
The method Choice.getChoice() is emitting some sort of bad code smell. I can't quite put my finger on it but the part about the abstract class knowing about the descendant classes Rock, Paper and Scissors just doesn't smell right to me.
Also, what is your reason for choosing Singleton? Do you see any advantage it has over the typesafe enum pattern?

[Junilu Lacar]

A point of consternation for me in this thread is this construct:
<pre>
public boolean beats(Choice choice) {
return choice.getName().equals("Scissors") ? true : false;
}
</pre>
which Ryo and Sandeep have used. Don't you find this redundant? BTW, that type of construct was listed in one article (sorry, I forgot the exact title) as one of the "Ways of Spotting a Newbie" programmer.
The expression
<pre>
choice.getName().equals("Scissors")
</pre>
already returns true or false so it is redundant to add "? true : false" just as the following
<pre>
if (booleanExpr)
return true;
else
return false;
</pre>
can be simply written as
<pre>
return (booleanExpr);
</pre>

Also, given the String reference s, it is much safer to do this:
"Literal".equals(s)
than it is to do this:
s.equals("Literal")

These are just a couple of basic idioms that I think a good Java programmer should know about and use.
[This message has been edited by JUNILU LACAR (edited June 17, 2001).]

[Wirianto Djunaidi]

Junilu,
I liked what you did with the second codes that you posted.
And I totally agree about the getName().equals() stuff.
I wasn't put much thought into it when I posted my code.
-Ryo

[Desai Sandeep]

Junilu,
I agree that the Conditional Operator (?: ) is redundant and should be removed.
I have used Singleton,since we donot require multiple instances of the Choice objects.
Designing Singleton over Polymorphism does lead to bad code smell, as it adds dependency of subtypes in the supertype.However, I believe it is OK in our case, since we have designed the supertype as abstract (in other words it means, the supertype is aware of the kind of instances it can hold)
So, Polymorphism would address the type safety and Singleton would ensure that only one instance having global access is created.
I am still working on how we could remove this "bad code smell" and yet design with Singleton - Any ideas?

- Sandeep
[This message has been edited by Desai Sandeep (edited June 18, 2001).]

[Frank Carver]

I still don't understand the need for a Singleton in this domain at all. Which concept in the problem will there only ever be one of, and why do we need to enforce that in the design?
I can maybe see the need for a factory, but even that seems too complicated for this simple exercise.

[Frank Carver]

I'm also a little worried by some of the assumptions in Junilu's "test case", which seem to overcomplicate the problem a bit:
Junilu writes:

public void testGameRules() {
    assertEquals(true, Choice.ROCK.beats(Choice.SCISSORS));
    assertEquals(false, Choice.ROCK.beats(Choice.ROCK));
    assertEquals(false, Choice.ROCK.beats(Choice.PAPER));
    assertEquals(true, Choice.SCISSORS.beats(Choice.PAPER));
    assertEquals(false, Choice.SCISSORS.beats(Choice.ROCK));
    assertEquals(false, Choice.SCISSORS.beats(Choice.SCISSORS));
    assertEquals(true, Choice.PAPER.beats(Choice.ROCK));
    assertEquals(false, Choice.PAPER.beats(Choice.SCISSORS));
    assertEquals(false, Choice.PAPER.beats(Choice.PAPER));
}

This introduces the need for a Choice class or classes and may tend to either spread the knowledge between several implementation classes, or encode it in a non-intuitive way as in the replies above.
I would much rather write the first test at a "higher" level (closer to the actual user story, such as it is), to allow a more flexible refactoring of the inner details. The user neither knows nor cares about symbolic classes, singletons, typesafe-enums or whatever, so how about something like:

public void testGameRules() 
{
    // getWinner returns 1 if the first parameter wins,
    // 2 if the second parameter wins, or 0 if nobody wins
    assertEquals(1, game.getWinner("rock", "scissors"));
    assertEquals(0, game.getWinner("rock", "rock"));
    assertEquals(2, game.getWinner("rock", "paper"));
    assertEquals(1, game.getWinner("paper", "rock"));
    assertEquals(0, game.getWinner("paper", "paper"));
    assertEquals(2, game.getWinner("paper", "scissors"));
    assertEquals(1, game.getWinner("scissors", "paper"));
    assertEquals(0, game.getWinner("scissors", "scissors"));
    assertEquals(2, game.getWinner("scissors", "rock"));
}

This then suggests (to me at least) a simpler style of directly encoding these results as data, rather than code:

class HandGame
{
  private static final Integer nobody = new Integer(0);
  private static final Integer first = new Integer(1);
  private static final Integer second = new Integer(2);
 
  static Map winners = new HashMap();
  static
  {
    winners.put("rock,scissors", first);
    winners.put("rock,rock", nobody);
    winners.put("rock,paper", second);
    winners.put("paper,rock", first);
    winners.put("paper,paper", nobody);
    winners.put("paper,scissors", second);
    winners.put("scissors,paper", first);
    winners.put("scissors,scissors", nobody);
    winners.put("scissors,rock", second);
  }
 
  public static int getWinner(String first, String second)
  {
    Integer answer = winners.get(first + "," + second);
    if (answer == null)
    {
      answer = nobody;
    }
 
    return answer.intValue();
  }
}

Any comments ?
[This message has been edited by Frank Carver (edited June 19, 2001).]

[Junilu Lacar]

Frank,
Isn't it interesting how many different ways this simple problem can be approached?
Your initial test is interesting. I see a test as a way to decide on how you want to use a class. Personally, I wouldn't write a getWinner() method for Game. I am just not comfortable with using literals like that.
With a problem as small as this, I guess it doesn't really matter. But with classes that may have more dependencies, can you imagine the kind of ripple effect you would have if the implementation somehow changed, for example, because of Internationalization/Localization? (whew! now I see why they shorten those to I18N and L10N)
This is why I think using some sort of abstraction, in the form of a static final class or variable, is generally a better choice.
It would be interesting though to see how the program and tests would develop going down your path.
Another observation: it doesn't really seem like any of our approaches are doing refactoring. It's more of a rewrite really. I wonder if we could really resist the urge to do that in the real world.
When I see nasty code I don't like, I usually end up rewriting the code from scratch. More often than not, that gets me in trouble especially if the code I'm trying to rewrite has a lot of hidden dependencies.
Perhaps this is not a very good example to try strict refactoring out on though. Oh well, let's get on with it anyway.

[Desai Sandeep]

Hi Junilu,
Yes, I was going to say, programmers like to start from scratch, if they happen to see a nasty code.They do more of redesigning than refactoring.
That said, I think we are forgotten a very critical factor over here - Time.Remember unlike real-time projects, we are not having any schedule to deliver.Had this been a constraint, I would have probably never thought of making an abstract class the way I did.Then may be I would have done some refactoring and sooner or latter would have suffered with some bloated and incohesive codes.
However, this exercise is very good, since we are actually having a post now which teases us to think in terms of assigning responsibilities to appropriate classes.
Thanks,
Sandeep
[This message has been edited by Desai Sandeep (edited June 20, 2001).]

[Desai Sandeep]

Frank,
My contention on Singleton is based on the fact that it is not relevant to have many Choice objects of Rock, Scissors and Paper.
I was thinking in lines of associating the abstract Choice class with the Player.The Player will have dependency on the Choice Objects.When he wants the reference of any of the Choice Objects he would ask the same from Choice abstract class.Had we not based the Choices on Singleton we would have had many instances of choices (Reference Objects).
The question is do we really want One type of choice to be distinguished from other?.If yes, then Singleton Pattern is not required.If not(since neither we want each Choice Object to have a unique identity nor do we want it to be a Value Object, i.e. Player handles the lifecycle of the Choice), hence I have designed it on Singleton Pattern.
In a way Junilu and Ryo, have done the same thing by defining the Choices as static final.But I believe making the Choice as abstract and then subtyping the choices allows the code to become more cohesive and reusable.

Please let me know your views on this.
Also, I realised implementing Singleton over Polymorphism can be a tricky exercise.So I would like to hear on some work arounds for this.
As regards Factory Pattern, in a way, we can say the polymorphic abstract class Choice will act as a factory for the individual choices.Maybe you can say the Choice is designed on the lines of "Factory" Pattern (as the clients will talk only with the abstract class - getChoice(name) the only method available for clients to obtain the Choices suggests that!) and "Singleton" (only one global instance of Rock,Scissors and Paper choices are available) Pattern.
Hope this makes sense.
Thanks,
Sandeep
[This message has been edited by Desai Sandeep (edited June 20, 2001).]

[Frank Carver]

I suppose I can't help but see refactoring in the light of "do the simplest thing which can possibly work". We don't know that the rock/paper/scissors game will ever need I18N or L10N, or, in fact that it will ever need to do anything other than what it does now. That's why I feel that introducing intermediate concepts may be a mistake.
Just as with other excercises here, we have no real customer to make priority decisions such as this, so we are just floating on our own opinions.
My original question about whether small changes to enhance testability was mainly about providing a side-path to avoid the randomness, so we can test for repeatable results. I'm extremely wary of starting the refactoring from the "inside" until we have tests in place to make sure that the "outside" remains the same.
I agree, though, that these challenges are always interesting and thought-provoking.

[Junilu Lacar]

A friend referred me to a site that had this article about programmers' seemingly innate desire to rewrite rather than refactor: http://www.joelonsoftware.com/stories/storyReader$47

[Desai Sandeep]

Hi,
With whatever discussions we had over here, all of us always opted for redesigning rather than refactoring.However,Martin in his book says redesigning gives short-term pain and hence programmer usually donot consider this strategy.This didnot happen with us!
So what are the reasons that forces a programmer not to get into refactoring and instead go for redesigning?
I thought time/schedule is a very important constraint.Frank has come out with another point, i.e. absence of real customers and proper tests.Can we stimulate a real-time situation and go for refactoring?
- Sandeep
[This message has been edited by Desai Sandeep (edited June 21, 2001).]

[Junilu Lacar]

Desai,
I've got some code that we could probably try to refactor. I'm thinking that it could be also related to the Strategy design pattern so I'll post the code in that thread. I'll have to do that later though, I'm at work.
Junilu

[Frank Carver]

I think our desire to redesign rather than refactor in these exercises results from two forces.
The first force is that typicaly we are presented with code which is acknowledged by the poster as less than perfect and are asked to refactor "in situ" rather adding a new feature to an existing reasonably well-factored solution. This almost always brings up the response "I wouldn't start from here". Typically we also don't have external code or test cases which use the supplied problem code, so there is no existing behaviour to maintain. I think this is, to a large degree, solvable. With a bit of thought, we could produce a piece of code with external users and test cases, and ask participants to show ways of refactoring to add one or more specific new features.
The second force is inherent to the medium we are using (the Java Ranch Bulletin Board). The example code is always very small, and almost always in a single file. This is not a good representation of a real O-O system, and for such small solutions, the "pain" of a redesign is typically very low, whereas the effort of deciding which micro-aspects of the original "buggy" system to preserve in a refactoring is quite high. I'm not sure how to solve this one, though. Any suggestions?

[Junilu Lacar]

I agree with you Frank, we need a bigger system to do some realistic refactoring. The code I was going to post was a variation of my solution to one of the Cattle Drive exercises on Servlets. But then I'm hesitant to do that now because the Cattle Drivers may not like it.
I thought about Bill Bozeman's JavaWorkshop.net but I haven't been there for a while and it seemed to be going kind of slow the last time I went. But if any of you guys are game, we could go and try to liven things up over there.
JavaRanch is just so addictive though. I spend most of my time combing the forums I like to hang out in for new messages. Heck, now I even find myself spending time in Meaningless Drivel. And then there's www.flingthecow.com. I got on the Best Players of the week list (470, with my son's help) last night and I just had to put JavaRanch in the name. God help me!