Fixes #330 #374
Fixes #330 #374
Conversation
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
| final Double f = constantFrom == null ? value(scope, from) : constantFrom; | ||
| final Double t = constantTo == null ? value(scope, to) : constantTo; | ||
| Double s = constantStep == null ? value(scope, step) : constantStep; |
There was a problem hiding this comment.
❌ New issue: Complex Method
LoopStatement.FloatBounded.runIn has a cyclomatic complexity of 16, threshold = 9
| public Object runIn(final IScope scope) throws GamaRuntimeException { | ||
| final Object[] result = new Object[1]; | ||
| final Integer f = constantFrom == null ? value(scope, from) : constantFrom; | ||
| final Integer t = constantTo == null ? value(scope, to) : constantTo; | ||
| Integer s = constantStep == null ? value(scope, step) : constantStep; | ||
| boolean shouldBreak = false; | ||
| if (f.equals(t)) { | ||
| loopBody(scope, f, result); | ||
| } else if (f > t) { | ||
| if (s > 0) { | ||
| if (stepDefined) { | ||
| loopBody(scope, f, result); | ||
| return result[0]; | ||
| } | ||
| s = -s; | ||
| } | ||
| for (int i = f, n = t; i >= n && !shouldBreak; i += s) { | ||
| FlowStatus status = loopBody(scope, i, result); | ||
| switch (status) { | ||
| case CONTINUE: | ||
| continue; | ||
| case BREAK, RETURN, DIE, DISPOSE: | ||
| shouldBreak = true; | ||
| break; | ||
| default: | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
❌ New issue: Complex Method
LoopStatement.IntBounded.runIn has a cyclomatic complexity of 16, threshold = 9
| IExpression from = getFacet(IKeyword.FROM); | ||
| IExpression to = getFacet(IKeyword.TO); | ||
| IExpression step = getFacet(IKeyword.STEP); | ||
| final boolean isBounded = from != null && to != null; | ||
| boolean isInt = isBounded && from.getGamlType() == Types.INT && to.getGamlType() == Types.INT | ||
| && (step == null || step.getGamlType() == Types.INT); |
There was a problem hiding this comment.
❌ New issue: Complex Method
LoopStatement has a cyclomatic complexity of 10, threshold = 9
| final Number f = constantFrom == null ? getFromExp(scope, from) : constantFrom; | ||
| final Number t = constantTo == null ? getFromExp(scope, to) : constantTo; | ||
| Number s = constantStep == null ? getFromExp(scope, step) : constantStep; |
There was a problem hiding this comment.
✅ No longer an issue: Bumpy Road Ahead
LoopStatement.Bounded.runIn is no longer above the threshold for logical blocks with deeply nested code
| public Object runIn(final IScope scope) throws GamaRuntimeException { | ||
| final Object[] result = new Object[1]; | ||
| final Integer f = constantFrom == null ? value(scope, from) : constantFrom; | ||
| final Integer t = constantTo == null ? value(scope, to) : constantTo; | ||
| Integer s = constantStep == null ? value(scope, step) : constantStep; | ||
| boolean shouldBreak = false; | ||
| if (f.equals(t)) { | ||
| loopBody(scope, f, result); | ||
| } else if (f > t) { | ||
| if (s > 0) { | ||
| if (stepDefined) { | ||
| loopBody(scope, f, result); | ||
| return result[0]; | ||
| } | ||
| s = -s; | ||
| } | ||
| for (int i = f, n = t; i >= n && !shouldBreak; i += s) { | ||
| FlowStatus status = loopBody(scope, i, result); | ||
| switch (status) { | ||
| case CONTINUE: | ||
| continue; | ||
| case BREAK, RETURN, DIE, DISPOSE: | ||
| shouldBreak = true; | ||
| break; | ||
| default: | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
ℹ New issue: Bumpy Road Ahead
LoopStatement.IntBounded.runIn has 3 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function
| final Double f = constantFrom == null ? value(scope, from) : constantFrom; | ||
| final Double t = constantTo == null ? value(scope, to) : constantTo; | ||
| Double s = constantStep == null ? value(scope, step) : constantStep; |
There was a problem hiding this comment.
ℹ New issue: Bumpy Road Ahead
LoopStatement.FloatBounded.runIn has 3 blocks with nested conditional logic. Any nesting of 2 or deeper is considered. Threshold is one single, nested block per function
| @@ -1,7 +1,6 @@ | |||
| /******************************************************************************************************* | |||
| * | |||
| * LoopStatement.java, in gama.core, is part of the source code of the GAMA modeling and simulation platform | |||
| * (v.2.0.0). | |||
| * LoopStatement.java, in gama.core, is part of the source code of the GAMA modeling and simulation platform (v.2.0.0). | |||
There was a problem hiding this comment.
✅ Getting better: Overall Code Complexity
The mean cyclomatic complexity decreases from 5.44 to 5.24, threshold = 4
|
while testing this PR I noticed that a from/to loop with from<to but a negative step ends up in an infinite loop. |
Seems to me like a normal behavior 🤔 |
then we have another problem because in the other direction we just iterate once and exit the loop. I'm making a unit test model for loops because it seems like there are a lot of inconsistencies here |
- some models are not passing the tests right now do to bugs/inconsistencies in the loop statement
| final Double f = constantFrom == null ? value(scope, from) : constantFrom; | ||
| final Double t = constantTo == null ? value(scope, to) : constantTo; | ||
| Double s = constantStep == null ? value(scope, step) : constantStep; |
There was a problem hiding this comment.
❌ New issue: Code Duplication
The module contains 4 functions with similar structure: LoopStatement.FloatBounded.FloatBounded,LoopStatement.FloatBounded.runIn,LoopStatement.IntBounded.IntBounded,LoopStatement.IntBounded.runIn
The problem came from the choice of the operator + at compilation times, because step was not used to compute the type of i ...
| @@ -333,8 +332,7 @@ protected IExpression createVarWithTypes(final String tag) { | |||
| } | |||
| } | |||
| } else if (hasFacet(FROM) && hasFacet(TO)) { | |||
| final IExpression expr = getFacetExpr(FROM); | |||
| if (expr != null) { t = expr.getGamlType(); } | |||
| t = GamaType.findCommonType(getFacetExpr(FROM), getFacetExpr(TO), getFacetExpr(STEP)); | |||
There was a problem hiding this comment.
✅ Getting better: Complex Method
createVarWithTypes decreases in cyclomatic complexity from 17 to 16, threshold = 9
| @@ -333,8 +332,7 @@ protected IExpression createVarWithTypes(final String tag) { | |||
| } | |||
| } | |||
| } else if (hasFacet(FROM) && hasFacet(TO)) { | |||
| final IExpression expr = getFacetExpr(FROM); | |||
| if (expr != null) { t = expr.getGamlType(); } | |||
| t = GamaType.findCommonType(getFacetExpr(FROM), getFacetExpr(TO), getFacetExpr(STEP)); | |||
There was a problem hiding this comment.
✅ Getting better: Bumpy Road Ahead
createVarWithTypes decreases from 4 to 3 logical blocks with deeply nested code, threshold is one single block per function
| @@ -1,7 +1,6 @@ | |||
| /******************************************************************************************************* | |||
| * | |||
| * StatementDescription.java, in gama.core, is part of the source code of the GAMA modeling and simulation platform | |||
| * . | |||
| * StatementDescription.java, in gama.core, is part of the source code of the GAMA modeling and simulation platform . | |||
There was a problem hiding this comment.
✅ No longer an issue: Overall Code Complexity
The mean cyclomatic complexity in this module is no longer above the threshold
Separates the int and float bounded definitions. Makes a loop where "from" > "to" at least execute once with the value of "from". Should fix #330.