refactoring cyclomatic complexity code smells #145
Conversation
There was a problem hiding this comment.
Although I assume good intentions and a genuine attempt to fix certain code smells, I unfortunately must conclude that this PR adds other smells in exchange, introduces probable bugs and breaks the build by removing the package/import declarations. In order to approve the PR, all these should be fixed and the missing license/copyright notices must be restored.
| public static boolean isCorrect(long value) { | ||
| return (MINVALUE <= value) && (value <= MAXVALUE); | ||
| } | ||
| private static final String[] REGION_DESCRIPTIONS = { |
There was a problem hiding this comment.
I'd rather use Map here to make it explicit instead of trusting array indices, think of what happens if one line is accidentally removed or the order is changed. Mapping will probably make toString a bit easier too, e.g. by removing the need to check if the given value falls within valid range.
| @@ -33,74 +33,52 @@ public class NavAidType { | |||
| * @param deviceType Device type value to Stringify. | |||
| * @return a text string describing the Nav Aid type | |||
There was a problem hiding this comment.
An array doesn't "return" anything and doesn't have deviceType parameter, as it is not a method. It simply holds values that can be retrieved by an index. This is also a rather simple private field, so perhaps just // NavAid mapping or such would be sufficient, if need be.
| "not used", | ||
| "not used", | ||
| "not used", | ||
| "not used", | ||
| "not used", | ||
| "not used", | ||
| "not used", |
There was a problem hiding this comment.
Now that we are into static analysis and fixing code smells: Don't Repeat Yourself (DRY).
This is basically just wasted memory and again could be easily avoided by using an explicit mapping.
| /* | ||
| * ShipType.java | ||
| * Copyright (C) 2015-2020 Lázár József, Joshua Sweaney | ||
| * | ||
| * This file is part of Java Marine API. | ||
| * <http://ktuukkan.github.io/marine-api/> | ||
| * | ||
| * Java Marine API is free software: you can redistribute it and/or modify it | ||
| * under the terms of the GNU Lesser General Public License as published by the | ||
| * Free Software Foundation, either version 3 of the License, or (at your | ||
| * option) any later version. | ||
| * | ||
| * Java Marine API is distributed in the hope that it will be useful, but | ||
| * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
| * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License | ||
| * for more details. | ||
| * | ||
| * You should have received a copy of the GNU Lesser General Public License | ||
| * along with Java Marine API. If not, see <http://www.gnu.org/licenses/>. | ||
| */ | ||
| package net.sf.marineapi.ais.util; | ||
|
|
||
| /** | ||
| * Checks the ship type for validity. | ||
| * | ||
| * @author Lázár József, Joshua Sweaney | ||
| */ |
There was a problem hiding this comment.
When working on code that you don't own, especially when using someone else's work for your own benefit, the licensing and copyright info shall never be removed or altered. In addition, the package declaration is also gone.
| handleRMCSentence((RMCSentence) s, p, sog, cog, d, t, mode); | ||
| } else if (s instanceof VTGSentence) { | ||
| handleVTGSentence((VTGSentence) s, sog, cog); | ||
| } else if (s instanceof GGASentence) { | ||
| handleGGASentence((GGASentence) s, p, d, t, fix); | ||
| } else if (s instanceof GLLSentence) { | ||
| handleGLLSentence((GLLSentence) s, p); |
There was a problem hiding this comment.
Ok, some more on code smells: re-assignment on parameters is generally considered a bad practice. Depending on your config, this is something that also PMD could warn you about.
Moreover, your intent is to make changes to variables declared in other scope, which is a side effect and thus also a bad practice (see orthogonality and Law of Demeter).
Finally, your approach doesn't work in Java because the parameters are passed in by-value instead of by-reference. Hence, assigning a new value to method parameter does not overwrite the variable declared in the calling scope.
| * @see net.sf.marineapi.provider.event.PositionListener | ||
| * @see net.sf.marineapi.provider.event.PositionEvent | ||
| * @see net.sf.marineapi.nmea.io.SentenceReader | ||
| */ |
| @@ -21,42 +21,67 @@ | |||
| package net.sf.marineapi.ais.util; | |||
|
|
|||
| /** | |||
| * Checks the positioning device type for validity. | |||
| * Provides functionality related to positioning devices. | |||
There was a problem hiding this comment.
How does the changed comment make an improvement here? As compared to the original, I find it now more abstract and vague, telling less about the purpose of this class.
refactored cyclomatic complexity code smell #144