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.

Sign up for GitHub

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

refactoring cyclomatic complexity code smells #145

Closed
wants to merge 2 commits into from
Closed

refactoring cyclomatic complexity code smells #145

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ccb178a
refactoring cyclomatic complexity code smells
Nov 15, 2023
eff5751
refactoring JUnitTestContainsTooManyAsserts code smells
Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 37 additions & 43 deletions src/main/java/net/sf/marineapi/ais/util/MMSI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,49 +28,43 @@
public class MMSI {

private static final long MINVALUE = 0;
private static final long MAXVALUE = 999999999;
private static final long MAXVALUE = 999999999;

/**
* Checks whether the MMSI value is correct, i.e. within valid range.
*
* @param value MMSI value to check
* @return true if the value is semantically correct.
*/
public static boolean isCorrect(long value) {
return (MINVALUE <= value) && (value <= MAXVALUE);
}
private static final String[] REGION_DESCRIPTIONS = {
Copy link
Owner

@ktuukkan ktuukkan Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"Ship group, coast station, or group of coast stations",
"SAR aircraft",
"Europe",
"North and Central America and Caribbean",
"Asia",
"Oceana",
"Africa",
"South America",
"Assigned for regional Use",
"Nav aids or craft associated with a parent ship",
"Invalid MMSI number"
};

/**
* Returns the origin associated with the MMSI number.
*
* @param value MMSI value to stringify
* @return A String describing the region of the transmitter
*/
public static String toString(long value) {
int firstDigit = (int)(value / 100000000L);
switch (firstDigit) {
case 0:
return "Ship group, coast station, or group of coast stations";
case 1:
return "SAR aircraft";
case 2:
return "Europe";
case 3:
return "North and Central America and Caribbean";
case 4:
return "Asia";
case 5:
return "Oceana";
case 6:
return "Africa";
case 7:
return "South America";
case 8:
return "Assigned for regional Use";
case 9:
return "Nav aids or craft associated with a parent ship";
default:
return "Invalid MMSI number";
}
}
/**
* Checks whether the MMSI value is correct, i.e. within valid range.
*
* @param value MMSI value to check
* @return true if the value is semantically correct.
*/
public static boolean isCorrect(long value) {
return (MINVALUE <= value) && (value <= MAXVALUE);
}

/**
* Returns the origin associated with the MMSI number.
*
* @param value MMSI value to stringify
* @return A String describing the region of the transmitter
*/
public static String toString(long value) {
int firstDigit = (int) (value / 100000000L);
if (0 <= firstDigit && firstDigit <= 9) {
return REGION_DESCRIPTIONS[firstDigit];
}
return REGION_DESCRIPTIONS[10];
}
}
118 changes: 48 additions & 70 deletions src/main/java/net/sf/marineapi/ais/util/NavAidType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,74 +33,52 @@ public class NavAidType {
* @param deviceType Device type value to Stringify.
* @return a text string describing the Nav Aid type
Copy link
Owner

@ktuukkan ktuukkan Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*/
static public String toString (int deviceType) {
switch (deviceType) {
case 0:
return "Default, Type of Aid to Navigation not specified";
case 1:
return "Reference point";
case 2:
return "RACON (radar transponder marking a navigation hazard)";
case 3:
return "Fixed structure off shore, such as oil platforms, wind farms, rigs";
case 4:
return "Spare, Reserved for future use";
case 5:
return "Light, without sectors";
case 6:
return "Light, with sectors";
case 7:
return "Leading Light Front";
case 8:
return "Leading Light Rear";
case 9:
return "Beacon, Cardinal N";
case 10:
return "Beacon, Cardinal E";
case 11:
return "Beacon, Cardinal S";
case 12:
return "Beacon, Cardinal W";
case 13:
return "Beacon, Port hand";
case 14:
return "Beacon, Starboard hand";
case 15:
return "Beacon, Preferred Channel port hand";
case 16:
return "Beacon, Preferred Channel starboard hand";
case 17:
return "Beacon, Isolated danger";
case 18:
return "Beacon, Safe water";
case 19:
return "Beacon, Special mark";
case 20:
return "Cardinal Mark N";
case 21:
return "Cardinal Mark E";
case 22:
return "Cardinal Mark S";
case 23:
return "Cardinal Mark W";
case 24:
return "Port hand Mark";
case 25:
return "Starboard hand Mark";
case 26:
return "Preferred Channel Port hand";
case 27:
return "Preferred Channel Starboard hand";
case 28:
return "Isolated danger";
case 29:
return "Safe Water";
case 30:
return "Special Mark";
case 31:
return "Light Vessel / LANBY / Rigs";
default:
return "not used";
}
}
static private String[] navAidTypes = {
"Default, Type of Aid to Navigation not specified",
"Reference point",
"RACON (radar transponder marking a navigation hazard)",
"Fixed structure off shore, such as oil platforms, wind farms, rigs",
"Spare, Reserved for future use",
"Light, without sectors",
"Light, with sectors",
"Leading Light Front",
"Leading Light Rear",
"Beacon, Cardinal N",
"Beacon, Cardinal E",
"Beacon, Cardinal S",
"Beacon, Cardinal W",
"Beacon, Port hand",
"Beacon, Starboard hand",
"Beacon, Preferred Channel port hand",
"Beacon, Preferred Channel starboard hand",
"Beacon, Isolated danger",
"Beacon, Safe water",
"Beacon, Special mark",
"Cardinal Mark N",
"Cardinal Mark E",
"Cardinal Mark S",
"Cardinal Mark W",
"Port hand Mark",
"Starboard hand Mark",
"Preferred Channel Port hand",
"Preferred Channel Starboard hand",
"Isolated danger",
"Safe Water",
"Special Mark",
"Light Vessel / LANBY / Rigs",
"not used"
};

/**
* Returns a text string for the NavAid.
*
* @param deviceType Device type value to Stringify.
* @return a text string describing the Nav Aid type
*/
static public String toString(int deviceType) {
if (deviceType >= 0 && deviceType < navAidTypes.length) {
return navAidTypes[deviceType];
}
return "not used";
}
}
93 changes: 59 additions & 34 deletions src/main/java/net/sf/marineapi/ais/util/PositioningDevice.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,42 +21,67 @@
package net.sf.marineapi.ais.util;

/**
* Checks the positioning device type for validity.
* Provides functionality related to positioning devices.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*
* @author Lázár József
*/
public class PositioningDevice {

/**
* Returns a text string for the EPFD.
*
* @param deviceType Device type value to Stringify.
* @return a text string describing the positioning device type
*/
static public String toString (int deviceType) {
switch (deviceType) {
case 0:
return "undefined device";
case 1:
return "GPS";
case 2:
return "GLONASS";
case 3:
return "combined GPS/GLONASS";
case 4:
return "Loran-C";
case 5:
return "Chayka";
case 6:
return "integrated navigation system";
case 7:
return "surveyed";
case 8:
return "Galileo";
case 15:
return "internal GNSS";
default:
return "not used";
}
}

private static final String[] DEVICE_TYPES = {
"undefined device",
"GPS",
"GLONASS",
"combined GPS/GLONASS",
"Loran-C",
"Chayka",
"integrated navigation system",
"surveyed",
"Galileo",
"not used",
"not used",
"not used",
"not used",
"not used",
"not used",
"not used",
Comment on lines +40 to +46
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"internal GNSS",
"not used",
"not used",
"not used",
"not used",
"not used",
"not used",
"not used",
"not used",
"not used",
"not used",
"not used",
"not used",
"not used",
"not used",
"not used"
};

/**
* Returns a text string for the EPFD.
*
* @param deviceType Device type value to stringify.
* @return a text string describing the positioning device type
*/
static public String toString(int deviceType) {
if (isValidDeviceType(deviceType)) {
return DEVICE_TYPES[deviceType];
}
return "not used";
}

/**
* Checks whether the device type value is correct.
*
* @param deviceType Device type value to check.
* @return true if the value is semantically correct.
*/
static private boolean isValidDeviceType(int deviceType) {
return 0 <= deviceType && deviceType < DEVICE_TYPES.length;
}
}
Loading
Loading