Duplicated Code

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

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!

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

Advertisement


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.

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

f@dzhttp://festini.device-zero.de

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.

This topic is closed to new replies.

Advertisement