Duplicated Code

Started by
12 comments, last by Stroppy Katamari 10 years, 8 months ago

Given the code has different boolean values for each condition, is it okay to leave the code the way it is or is it duplicated code and needs fixing? Code is in Java.

Edit: I think I should have said this code is for a Tetris game. So the boolean values will be used in the update and draw method.


        if(e.getKeyCode() == KeyEvent.VK_RIGHT && keyCount == 1)
        {
            firstForm = false;
            secondForm = true;
            thirdForm = false;
            fourthForm = false;
        }
        else if(e.getKeyCode() == KeyEvent.VK_RIGHT && keyCount == 2)
        {
            firstForm = false;
            secondForm = false;
            thirdForm = true;
            fourthForm = false;
        }
        else if(e.getKeyCode() == KeyEvent.VK_RIGHT && keyCount == 3)
        {
            firstForm = false;
            secondForm = false;
            thirdForm = false;
            fourthForm = true;
        }
        else if(e.getKeyCode() == KeyEvent.VK_RIGHT && keyCount == 4)
        {
            firstForm = true;
            secondForm = false;
            thirdForm = false;
            fourthForm = false;
        }
Advertisement

Maybe this is cleaner?

if(e.getKeyCode() == KeyEvent.VK_RIGHT) {
        secondForm = keyCount == 1;
        thirdForm     = keyCount == 2;
        fourthForm   = keyCount == 3;
        firstForm      = keyCount == 4;
}

Are those form booleans mutually exclusive? In that case it would be better to use an int to represent the current form. And if they are not, an array would probably lead to further reductions in code repetition (don't name something that is actually numbered like first, second, third... or x, y, z... use an array instead)

You could set all forms to false, then only set one thing to true in the if blocks.

firstForm = false;
secondForm = false;
thirdForm = false;
fourthForm = false;
 
if(e.getKeyCode() == KeyEvent.VK_RIGHT && keyCount == 1)
        {
            secondForm = true;
        }
        else if(e.getKeyCode() == KeyEvent.VK_RIGHT && keyCount == 2)
        {
            thirdForm = true;
        }
        else if(e.getKeyCode() == KeyEvent.VK_RIGHT && keyCount == 3)
        {
            fourthForm = true;
        }
        else if(e.getKeyCode() == KeyEvent.VK_RIGHT && keyCount == 4)
        {
            firstForm = true;
        }

But I think it is fine to leave it as is, if it works as intended.

The correct answer is to refactor four boolean variables into a single integer indicating which form is active:

private int currentForm = 0; // "firstForm" active
private int countForm = 4;
 
public void onKeyPressed( Event e ) {
    KeyEvent k = e.getkeyCode();
    if ( k == KeyEvent.VK_RIGHT )
        currentForm = (currentForm+1)%countForm; // Modulus wraps "fourthForm" to "firstForm"
    else if ( k == KeyEvent.VK_LEFT )
        currentForm = (currentForm+countForm-1)%countForm; // Adition and modulus wraps "firstForm to "fourthForm"
}

Maybe this is cleaner?


if(e.getKeyCode() == KeyEvent.VK_RIGHT) {
        secondForm = keyCount == 1;
        thirdForm     = keyCount == 2;
        fourthForm   = keyCount == 3;
        firstForm      = keyCount == 4;
}

wait a second, you cannot assign an integer to a boolean.. ohmy.png

In professional games I'm used to a keyboard mapping. You press a key and it translates to a virtual event.

For example, is it possible to remap to a WASD configuration? Or IJKL configuration? Or map it to specialized keys of a gaming keypad?

So my recommended remapping would be something along the lines of:

for( int i=0; i<keyMapping.count; i++) {

if( k == keyMapping.key ) {

Event::BroadcastEvent(Events.KeyPressedEvent, keyMapping.event);

}

}

If the key generates an "Next Form" event (whatever that means in your game) then any system that cares about the event can handle it.

If the code is real, then you should be using an enum instead of 4 bools.

If I had to use bools, I'd rewrite it like this:
if(e.getKeyCode() == KeyEvent.VK_RIGHT)
{
	firstForm = false;
	secondForm = false;
	thirdForm = false;
	fourthForm = false;
	
	if(keyCount == 1)      firstForm  = true;
	else if(keyCount == 2) secondForm = true;
	else if(keyCount == 3) thirdForm  = true;
	else if(keyCount == 4) fourthForm = true;
}
With enums, I'd write it like this:
enum class Form {None = 0, First, Second, Third, Fourth};

if(e.getKeyCode() == KeyEvent.VK_RIGHT)
{
	form = Form::None;
	
	if(keyCount == 1)      form = Form::First;
	else if(keyCount == 2) form = Form::Second;
	else if(keyCount == 3) form = Form::Third;
	else if(keyCount == 4) form = Form::Fourth;
}
But I'd give the enumerations more descriptive names.

+1 to belfegor's method, which I also use occasionally.

Are those form booleans mutually exclusive? In that case it would be better to use an int to represent the current form. And if they are not, an array would probably lead to further reductions in code repetition (don't name something that is actually numbered like first, second, third... or x, y, z... use an array instead)

The boolean of each condition happens not only in keyPressed method but also update and draw method. It is for a Tetris game. So when a certain form of the piece has been activated, the other flags for other forms of that corresponding piece are turned off.

Maybe this is cleaner?


if(e.getKeyCode() == KeyEvent.VK_RIGHT) {
        secondForm = keyCount == 1;
        thirdForm     = keyCount == 2;
        fourthForm   = keyCount == 3;
        firstForm      = keyCount == 4;
}
wait a second, you cannot assign an integer to a boolean.. ohmy.png

You can*, but he's not. He's assigning the result of the equation (a == b). The == operator (which is a function with two parameters), returns a boolean. Same thing with < > !=, &&, ||, etc...

if() statements take booleans as a parameter.

*bool myBool = int(27); Try to avoid it though!

This topic is closed to new replies.

Advertisement