Jump to content
  • Advertisement
Sign in to follow this  
Nicholas Kong

Duplicated Code

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

If you intended to correct an error in the post then please contact us.

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 this post


Link to post
Share on other sites
Advertisement

Maybe this is cleaner?

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

Share this post


Link to post
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 this post


Link to post
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 this post


Link to post
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 this post


Link to post
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.. ohmy.png

Share this post


Link to post
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 )  {

    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.

Share this post


Link to post
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 this post


Link to post
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 this post


Link to post
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.. 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!

Edited by Servant of the Lord

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!