HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Best way to get rid of code repetition?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
ridwaygetrepetitioncodebest

Problem

I currently have this bit of code, but I need to repeat it for red green and blue. Is there a way I can do it without copying and pasting the code 3 times?

yellow.setOnClickListener(new View.OnClickListener() {    
    public void onClick (View v)   {   

        switch (buttonCount) {
        case 1:  
            empty1.setImageResource(R.drawable.yellow);
            buttonCount++;
            guess1= Colour.YELLOW;
            break;
        case 2:  
            empty2.setImageResource(R.drawable.yellow);
            buttonCount++;
            guess2=Colour.YELLOW;
            break;
        case 3:  
            empty3.setImageResource(R.drawable.yellow);
            buttonCount++;
            guess3=Colour.YELLOW;
            break;
        case 4:  
            empty4.setImageResource(R.drawable.yellow);
            buttonCount++;
            guess4=Colour.YELLOW;
            break;
        case 5:  
            empty5.setImageResource(R.drawable.yellow);
            buttonCount++;
            guess5=Colour.YELLOW;
            break;

        }
    }
});

Solution

If you look at your naming schema, you have emptyX and guessX, where X is an index. This is an indicator that you should be using an array or a list:

EmptyType empties[5] = { empty1, empty2, ... };
GuessType guesses[5] = { guess1, guess2, ... };

yellow.setOnClickListener(new View.OnClickListener() {
    public void onClick (View v) {
        assert buttonCount < 5;
        empties[buttonCount - 1].setImageResource(R.drawable.yellow);
        buttonCount++;
        guesses[buttonCount - 1] = Colour.YELLOW;
    }
})


If you have to do the same thing for three colours, either copy&paste this small snippet, or extract it into another method, which you can call with appropriate colours:

private void doStuff(EventSource source, Color color, ImageResource resource) {
  source.setOnClickListener(new View.OnClickListener() {
    public void onClick (View v) {
        assert buttonCount < 5;
        empties[buttonCount - 1].setImageResource(resource);
        buttonCount++;
        guesses[buttonCount - 1] = color;
    }
  });
}

  doStuff(yellow, Color.YELLOW, R.drawable.yellow);
  doStuff(blue,   Color.BLUE,   R.drawable.blue  );
  doStuff(green,  Color.GREEN,  R.drawable.green );


This still looks like you could choose better data structures, so that the Color and the corresponding ImageResource and EventSource are explicitly connected.

Code Snippets

EmptyType empties[5] = { empty1, empty2, ... };
GuessType guesses[5] = { guess1, guess2, ... };

yellow.setOnClickListener(new View.OnClickListener() {
    public void onClick (View v) {
        assert buttonCount < 5;
        empties[buttonCount - 1].setImageResource(R.drawable.yellow);
        buttonCount++;
        guesses[buttonCount - 1] = Colour.YELLOW;
    }
})
private void doStuff(EventSource source, Color color, ImageResource resource) {
  source.setOnClickListener(new View.OnClickListener() {
    public void onClick (View v) {
        assert buttonCount < 5;
        empties[buttonCount - 1].setImageResource(resource);
        buttonCount++;
        guesses[buttonCount - 1] = color;
    }
  });
}

  doStuff(yellow, Color.YELLOW, R.drawable.yellow);
  doStuff(blue,   Color.BLUE,   R.drawable.blue  );
  doStuff(green,  Color.GREEN,  R.drawable.green );

Context

StackExchange Code Review Q#35986, answer score: 11

Revisions (0)

No revisions yet.