Duplicated Code

This topic is 1658 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

Recommended Posts

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;
}
Edited by warnexus

Share on other sites

Maybe this is cleaner?

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


Share on other sites

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)

Share on other sites

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.

Share on other sites

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"
}


Share on other sites

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..

Share on other sites

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 )  {

}

}

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.

Share on other sites
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.

Share on other sites

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.

Edited by warnexus

Share on other sites

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..

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!

Edited by Servant of the Lord

Share on other sites

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..

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!

Oh I see he should include the paratheses. It was hard to see without the paratheses. Thanks for the additional information I needed!

Edited by warnexus

Share on other sites

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

Yeah, I think I should leave it the way it is since the boolean values are mutually exclusive.

Share on other sites

Ask yourself this: if instead of describing forms (which are basically just 90° steps of rotation) you would describe a free rotation, would you go and create 360 bools all called rotated1Degree, rotated2Degree, ...? What if you need half or quarter degree steps? Because that's essentially what you are doing right now, except it hasn't yet reached the point of becoming obviously ridiculous and unmaintainable.

In terms of getting the project done: it works, move on.

In terms of practice or better code: consider some of the cleaner approaches mentioned above.

How are you doing all the other stuff like drawing? Is there a lot if if-else to pick the right shape? Wouldn't it be easier if the form was a number that also just indexes into an array with the correct shape? Are you checking for consistency (always one and exactly one of them being true)? Basically when you say "mutually exclusive" that should also mean "not stored as multiple bools".

Edited by Trienco

Share on other sites

Yeah, I think I should leave it the way it is since the boolean values are mutually exclusive.

That makes no sense. The fact the booleans are mutually exclusive is exactly why they should be removed. They are breaking the DRY principle ("do not repeat yourself") because they represent the same data. Whenever xyzForm == true, the others must be false, but keeping booleans means you have to spell that out every time and it's possible to make a mistake.

Whenever you have a small, fixed set of possible states for something, and exactly one of them is active at any one time, that's pretty much what enums are for.

To encapsulate the possible transitions, I'd write "rotateLeft", "rotateRight" etc. functions inside the enum. For an example, see: http://stackoverflow.com/a/2647709
If you need a 0..3 integer representation for something like indexing into an array, you can always get that out of the enum with .ordinal(). (You'll see people claiming that you shouldn't rely on enum ordinals, but the reasons have to do with situations where more enum values might be added in the middle of the list; that's not an issue in this case since no more unique rotations exist.)

Keeping a raw integer between 0..3, like in fastcall22's example, would also work. But also for that I would definitely write separate manipulation functions that make sure the value stays in the right range, instead of sprinkling that logic inside UI code.