We Are Going To Discuss About SonarQube Refactor this method to reduce its Cognitive Complexity. So lets Start this Java Article.
SonarQube Refactor this method to reduce its Cognitive Complexity
- SonarQube Refactor this method to reduce its Cognitive Complexity
I don't think that merging many
if
conditions to one or simply do a code clean up, for example by changing the order of some instructions, can solve your problem. - SonarQube Refactor this method to reduce its Cognitive Complexity
I don't think that merging many
if
conditions to one or simply do a code clean up, for example by changing the order of some instructions, can solve your problem.
Solution 1
I don’t think that merging many if
conditions to one or simply do a code clean up, for example by changing the order of some instructions, can solve your problem.
Your code does not match the single responsibility principle. You should refactor this big method to smaller parts. Due to this it will testable, easier to maintain and read. I spent some time and did this:
public static boolean isWrapperValid(WrapperClass wrapper, boolean isTechnicalToken) {
final WrapperClass unpackedWrapper = unpackWrapper(wrapper);
boolean wrapperValid = isUnpackedWrapperValid(unpackedWrapper);
Key key = null;
try {
key = unpackedWrapper.getKey();
} catch (final Exception exception) {
return wrapperValid;
}
if (key != null) {
if (doesKeyMeetsBasicConditions(key)) {
return wrapperValid;
}
if (doesKeyMeetsValueConditions(key)) {
return true;
}
}
return wrapperValid;
}
protected static WrapperClass unpackWrapper(final WrapperClass wrapper) {
if (wrapper != null && wrapper.length() > 7 && wrapper.substring(0, 6).equalsIgnoreCase("XYZ")) {
return wrapper.substring(7, wrapper.lastIndexOf('.') + 1);
}
return wrapper;
}
protected static boolean isUnpackedWrapperValid(final WrapperClass wrapper) {
return wrapper != null && wrapper.equalsIgnoreCase("TFR");
}
protected static boolean doesKeyMeetsBasicConditions(final Key key) {
Date tokenExpiryTime = key.getExpiresAt();
if (tokenExpiryTime != null) {
return true;
}
String algorithm = key.getAlgorithm();
if (!DESIRED_ALGO.equals(algorithm)) {
return true;
}
String value6 = key.getType();
return !DESIRED_TYPE.equals(value6);
}
protected static boolean doesKeyMeetsValueConditions(final Key key) {
return key.getValue1() != null && key.getValue2().size() > 0
&& key.getValue3() != null && key.getValue4() != null
&& key.getValue5() != null;
}
I don’t know the domain logic, so some of my methods have stupid names etc. As you can see, now you have a lot of smaller methods with not many branches (if
conditions) – easier to test (a static code is not nice, but you can mock it by using for example PowerMock).
Original Author agabrys Of This Content
Solution 2
A bit of rewriting delivered a simplification, that still could be improved upon.
public static boolean isWrapperValid(WrapperClass wrapper, boolean isTechnicalToken){
if (wrapper != null && wrapper.length() > 7
&& wrapper.substring(0, 6).equalsIgnoreCase("XYZ"))
{
wrapper = wrapper.substring(7, wrapper.lastIndexOf('.')+1);
}
boolean isValidWrapper = wrapper != null && wrapper.equalsIgnoreCase("TFR");
try {
String key = wrapper.getKey();
if (key != null && key.getExpiresAt() == null
&& DESIRED_ALGO.equals(key.getAlgorithm())
&& DESIRED_TYPE.equals(key.getType())
&& key.getValue1() != null && !key.getValue2().isEmpty()
&& key.getValue3() != null && key.getValue4() != null
&& key.getValue5() != null) {
isValidWrapper = true;
}
}
catch (Exception exception) {
// DO NOTHING
}
return isValidWrapper;
}
After comment: here I catch any exception for all calls.
Original Author Joop Eggen Of This Content
Solution 3
First of all, Sonar should give you more flags: reusing the wrapper
parameter is usually a bad practice, NPE where invoking wrapper.getKey
because wrapper
can be null, but anyway, not the point…
Try reducing the number of if
statements by creating local boolean variables (or possibly 1 big if
statement if you have less than 5 or 6 tests, but often less readable). Once it’s done, you should only have 1 block testing these boolean variables, and have one return statement, like the example above (not necessarily accurate!):
boolean expired = tokenExpiryTime != null;
boolean desiredAlgo = DESIRED_ALGO.equals(key.getAlgorithm());
boolean desiredType = DESIRED_TYPE.equals(value6);
if (expired || !desiredAlgo || !desiredType) {
return isValidWrapper;
}
However, your Cognitive complexity level seems pretty low if this kind of algorithm triggers it…
Another big way to reduce an algorithm complexity is to turn sub-blocks of code (loops, if and try-catch) into private methods. In your example, it could be something like a checkWrapperValidity
method, responsible for every test returning isValidWrapper
Original Author HBo Of This Content
Conclusion
So This is all About This Tutorial. Hope This Tutorial Helped You. Thank You.