HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Functional interface uses uncheck or unsafe operations

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
operationsusesunsafeinterfacefunctionaluncheck

Problem

First of all, I am absolutely surprised that this code even works. Originally what I did was create a version checking system for Minecraft plugins (specifically Bukkit, Craftbukkit, & Spigot).

What I wanted to do was refactor my Version class in order to make it platform independent (where it doesn't depend or provide functionality for only Minecraft servers and plugins).

So instead of the constructors accepting Bukkit classes, I changed it to accept a generic String that would represent the version. Then I made Builder methods, which I deleted and move into a factory: VersionFactory (this makes it easier for end-users to construct Version objects specific to Bukkit servers).

So, the main methods of Version.java are isCompatible() and isSupported(). However, the first thing these methods did was perform an isEnabled() check on the Version object because any compatibility checks should fail (as intended) if the target plugin isn't even installed on the server.

However, any such checks on specific Bukkit classes and objects would restrict the use of my Version checking system to only Bukkit. And I wanted to make it platform independent.

So what I did was create a functional interface called Tester, and asked for it in the constructor of Version (along with the object under test). The primary intent is to allow plugin.isEnabled() to be run and return true or false BEFORE isCompatible() or isSupported() is executed. However, it does allow ANY custom test() to be performed.

What is surprising is that I can pass type Object into the constructor, which gets assigned to a field, and ultimately gets passed to tester.isEnabled() which is able to call a plugin specific method on a generic Object type!

Version.java

```
package mc.euro.version;

import java.util.regex.Matcher;
import java.util.regex.Pattern;

/**
* The Version object: Capable of asking the important questions:.
*
* Is the version that's currently installed on the server comp

Solution

if (x >= 0) {
        return true;
    } 
    return false;

    if (x <= 0) {
        return true;
    }
    return false;

    if (matcher.find()) {
        return true;
    }
    return false;


Are constructs I'd express as

return x >= 0;


and

return x <= 0;


and

return matcher.find();


instead.

@Override
public int compareTo(Version whichVersion) {
    int[] currentVersion = parseVersion(this.version);
    int[] otherVersion = parseVersion(whichVersion.toString());
    int length = (currentVersion.length >= otherVersion.length) ? currentVersion.length : otherVersion.length;
    for (int index = 0; index  otherVersion.length) {
                return currentVersion[index] - 0;
            } else if (currentVersion.length < otherVersion.length) {
                return 0 - otherVersion[index];
            }
        }
    }
    return 0;
}


index = index + 1 can be index++.

for return currentVersion[index] - 0;, it's silly to subtract 0 as it does nothing.

index = otherVersion.length) ? currentVersion.length : otherVersion.length;, consider using Math.max(currentVersion.length, otherVersion.length) instead.

The reason I would express all these statements differently is because most of those things can simply be expressed in less code. The statement index <= (length - 1) is not easier to understand, nor does it provide any semantic value over index < length. By removing these overly complex syntactical structures, you make it easier to understand the code.

Code Snippets

if (x >= 0) {
        return true;
    } 
    return false;

    if (x <= 0) {
        return true;
    }
    return false;

    if (matcher.find()) {
        return true;
    }
    return false;
return x >= 0;
return x <= 0;
return matcher.find();
@Override
public int compareTo(Version whichVersion) {
    int[] currentVersion = parseVersion(this.version);
    int[] otherVersion = parseVersion(whichVersion.toString());
    int length = (currentVersion.length >= otherVersion.length) ? currentVersion.length : otherVersion.length;
    for (int index = 0; index <= (length - 1); index = index + 1) {
        try {
            if (currentVersion[index] != otherVersion[index]) {
                return currentVersion[index] - otherVersion[index];
            }
        } catch (IndexOutOfBoundsException ex) {
            if (currentVersion.length > otherVersion.length) {
                return currentVersion[index] - 0;
            } else if (currentVersion.length < otherVersion.length) {
                return 0 - otherVersion[index];
            }
        }
    }
    return 0;
}

Context

StackExchange Code Review Q#64607, answer score: 5

Revisions (0)

No revisions yet.