Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Check: AvoidFinalConditionInversion #112

Closed
romani opened this issue Apr 14, 2013 · 8 comments
Closed

New Check: AvoidFinalConditionInversion #112

romani opened this issue Apr 14, 2013 · 8 comments
Labels

Comments

@romani
Copy link
Member

@romani romani commented Apr 14, 2013

We need a check that could catch "not" operator - "!" (an exclamation mark) that is used as final operation, but it is possible without changing a meaning of condition change its parts to make condition more readable.

Bad code examples:
isDifferent(): return ! (a == b); ==should be==> return a != b;
isLower(): boolean c = !(a > b); ==should be==> boolean c = a <= b;
isLowerLimits(): if (! ((a>=8) && (b>=5)) ) ==should be==> if ((a<8) || (b<5))

Big example (not sure we could catch such cases):

return !(timeLoadMlls < maxMasterSymbolLoadStampMills 
          && fileName.toLowerCase().contains(MASTER_SYMBOL_NAME_PART) 
          || timeLoadMlls < maxOtherLoadStampMills 
          && !fileName.toLowerCase().contains(MASTER_SYMBOL_NAME_PART));

==should be==>

return (timeLoadMlls >= maxMasterSymbolLoadStampMills 
           || !fileName.toLowerCase().contains(MASTER_SYMBOL_NAME_PART) 
           && timeLoadMlls >= maxOtherLoadStampMills 
           || fileName.toLowerCase().contains(MASTER_SYMBOL_NAME_PART));
@ghost ghost assigned ilumar Apr 30, 2013
@daniilyar daniilyar added difficult and removed enhancement labels Aug 15, 2014
@alexkravin
Copy link
Contributor

@alexkravin alexkravin commented Sep 29, 2014

It would be a good idea to insert "how to avoid" advice into violation message, but sure it's impossible to cover all cases, so Check has to put violation like (Condition inversion could be avoided.)

@alexkravin
Copy link
Contributor

@alexkravin alexkravin commented Sep 29, 2014

That should be done in: return, loops and if conditions, right?

@romani
Copy link
Member Author

@romani romani commented Sep 29, 2014

Message should be simple, do not propose solution.

Yes, loops, if , returns should be checked.

Beware that check might find some very debatable cases, please run it on big projects and try to make sure that code would become better after required changes (some additional options might be required)

@alexkravin
Copy link
Contributor

@alexkravin alexkravin commented Sep 30, 2014

romani added a commit that referenced this issue Nov 29, 2014
@romani
Copy link
Member Author

@romani romani commented Nov 29, 2014

implemented by Alexey.

@romani romani closed this Nov 29, 2014
@rdiachenko
Copy link
Member

@rdiachenko rdiachenko commented Jun 25, 2015

if (! ((a>=8) && (b>=5)) ) ==should be==> if ((a<8) && (b<5))


return !(timeLoadMlls < maxMasterSymbolLoadStampMills
&& fileName.toLowerCase().contains(MASTER_SYMBOL_NAME_PART)
|| timeLoadMlls < maxOtherLoadStampMills
&& !fileName.toLowerCase().contains(MASTER_SYMBOL_NAME_PART));
==should be==>
return (timeLoadMlls >= maxMasterSymbolLoadStampMills
&& fileName.toLowerCase().contains(MASTER_SYMBOL_NAME_PART)
|| timeLoadMlls >= maxOtherLoadStampMills
&& !fileName.toLowerCase().contains(MASTER_SYMBOL_NAME_PART));

According to De Morgan's lows the resulted expressions should be as follows:

if (! ((a>=8) && (b>=5)) ) ==should be==> if ((a<8) || (b<5))

and

return !(timeLoadMlls < maxMasterSymbolLoadStampMills 
&& fileName.toLowerCase().contains(MASTER_SYMBOL_NAME_PART) 
|| timeLoadMlls < maxOtherLoadStampMills 
&& !fileName.toLowerCase().contains(MASTER_SYMBOL_NAME_PART));

==should be==>

return (timeLoadMlls >= maxMasterSymbolLoadStampMills 
|| !fileName.toLowerCase().contains(MASTER_SYMBOL_NAME_PART) 
&& timeLoadMlls >= maxOtherLoadStampMills 
|| fileName.toLowerCase().contains(MASTER_SYMBOL_NAME_PART));
@romani
Copy link
Member Author

@romani romani commented Jun 25, 2015

description was updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.