[#IDEADEV-5669] 'if can be simplified' is now too smart

IDEA: Development

'if can be simplified' is now too smart

Created: 20 Apr 06 16:02 Updated: 06 Nov 08 23:34

Component/s:

Code Analysis. Inspection

Fix Version/s:

Undefined

Original Estimate:

Unknown

Remaining Estimate:

Unknown

Time Spent:

Unknown

Build:	5,218
Severity:	Medium

Description

« Hide

this code

if (calendar.get(Calendar.HOUR_OF_DAY) < from.getHours())
return false;
if (calendar.get(Calendar.HOUR_OF_DAY) == from.getHours() && calendar.get(Calendar.MINUTE) < from.getMinutes())
return false;
if (calendar.get(Calendar.HOUR_OF_DAY) > till.getHours())
return false;
return !(calendar.get(Calendar.HOUR_OF_DAY) == till.getHours() && calendar.get(Calendar.MINUTE) > till.getMinutes());

is highlighted to be simplified

but result [which is 100% correct]

return calendar.get(Calendar.HOUR_OF_DAY) >= from.getHours() && !(calendar.get(Calendar.HOUR_OF_DAY) == from.getHours() && calendar.get(Calendar.MINUTE) <
from.getMinutes()) && calendar.get(Calendar.HOUR_OF_DAY) <= till.getHours() && !(calendar.get(Calendar.HOUR_OF_DAY) == till.getHours() &&
calendar.get(Calendar.MINUTE) > till.getMinutes());

does not look very simple for me. maybe we need to limit those simplifications?

All

Comments

Work Log

Change History

Sort Order:

[ Permlink | « Hide ]

Bas Leijdekkers - 20 Apr 06 16:47

Agreed, the inspection/quick fix does not simplify. In the latest build I have already relegated the inspection to the J2ME category. Perhaps I could also give it a less misleading name. Or do you have a suggestion on how to limit the inspection?

[ Permlink | « Hide ]

andrey belomutskiy - 20 Apr 06 16:54

what is the problem? the problem we can get huge logical expressions

so let's limit resulting expression operands number? i.e. if resulting 'simplified' expression have more than say 20 operands - do not offer such 'simplification'

[ Permlink | « Hide ]

Bas Leijdekkers - 20 Apr 06 17:57

Even not so huge logical expressions are much less clear than the original if statements which were replaced. I have decided to rename it "If statement may be replaced by boolean expression", because the inspection just does not simplify.

Number of operands does sound like a reasonable limiter. I will think about adding an option to the inspection. 20 sounds a bit high though, your "simple" expression has only 12. I'll make it configurable with some reasonable default.

[ Permlink | « Hide ]

andrey belomutskiy - 20 Apr 06 17:59

i do confess - i was too lazy to count