SpotBugs (4.9.8) Analysis for app (spotbugsGplayDebug)

SEI CERT rule OBJ01-J requires that accessibility to fields must be limited. Otherwise, the values of the fields may be manipulated from outside the class, which may be unexpected or undesired behaviour. In general, requiring that no fields are allowed to be public is overkill and unrealistic. Even the rule mentions that final fields may be public. Besides final fields, there may be other usages for public fields: some public fields may serve as "flags" that affect the behavior of the class. Such flag fields are expected to be read by the current instance (or the current class, in case of static fields), but written by others. If a field is both written by the methods of the current instance (or the current class, in case of static fields) and from the outside, the code is suspicious. Consider making these fields private and provide appropriate setters, if necessary. Please note that constructors, initializers and finalizers are exceptions, if only they write the field inside the class, the field is not considered as written by the class itself.

See CWE-766: Critical Data Element Declared Public.

This method uses specific collection interface/classes for parameters when only methods defined in the Collection class are used. Consider increasing the abstraction of the interface to make low impact changes easier to accomplish in the future.

Take the following example:

private void printList(List<String> list) {
    for (String s : list) {
        System.out.println(s);
    }
}

private void printList(Collection<String> list) {
    ...

IDEs tend to have tools to help generalize parameters. For example, in Eclipse, the refactoring tool Generalize Declared Type helps find an appropriate level of concreteness.

This class is an inner class, but does not use its embedded reference to the object which created it.  This reference makes the instances of the class larger, and may keep the reference to the creator object alive longer than necessary.  If possible, the class should be made static.

The return value from a method is dereferenced without a null check, and the return value of that method is one that should generally be checked for null. This may lead to a NullPointerException when the code is executed.

This method uses an integer-based for loop to iterate over a java.util.List, by calling List.get(i) each time through the loop. The integer is not used for other reasons. It is better to use an Iterator instead, as depending on List implementation, iterators can perform better, and they also allow for exchanging of other collection types without issue.

This class uses an ordinary set or map collection and uses an enum class as the key type. It is more performant to use the JDK 1.5 EnumSet or EnumMap classes.

The behavior of the String(byte[] bytes) and String.getBytes() is undefined if the string cannot be encoded in the platform's default charset. Instead, use the String(byte[] bytes, String encoding) or String.getBytes(String encoding) constructor which accepts the string's encoding as an argument. Be sure to specify the encoding used for the user's locale.

As per the Java specifications, "UTF-8", "US-ASCII", "UTF-16" and "ISO-8859-1" will all be valid encoding charsets. If you aren't sure, try "UTF-8".

New in Java 1.7, you can specify an encoding from StandardCharsets, like StandardCharsets.UTF_8. These are generally preferrable because you don't have to deal with UnsupportedEncodingException.

A public static method returns a reference to a mutable object or an array that is part of the static state of the class. Any code that calls this method can freely modify the underlying array. One fix is to return a copy of the array.

This method makes extensive use of methods from another class over methods of its own class. Typically this means that the functionality that is accomplished by this method most likely belongs with the class that is being used so liberally. Consider refactoring this method to be contained in that class, and to accept all the parameters needed in the method signature.

This method uses concrete classes for parameters when only methods defined in an implemented interface or superclass are used. Consider increasing the abstraction of the interface to make low impact changes easier to accomplish in the future.

Take the following example:

private void appendToList(ArrayList<String> list) {
    if (list.size() < 100) {
        list.add("Foo");
    }
}

private void appendToList(List<String> list) {
    ...

IDEs tend to have tools to help generalize parameters. For example, in Eclipse, the refactoring tool Generalize Declared Type helps find an appropriate level of concreteness.

This method calls algorithmic operations on a String that was returned from a toString() method. As these methods are for debugging/logging purposes, it shouldn't be the basis of core logic in your code.

This method creates and uses a java.io.FileInputStream or java.io.FileOutputStream object. Unfortunately both of these classes implement a finalize method, which means that objects created will likely hang around until a full garbage collection occurs, which will leave excessive garbage on the heap for longer, and potentially much longer than expected. Java 7 introduced two ways to create streams for reading and writing files that do not have this concern. You should consider switching from these above classes to InputStream is = java.nio.file.Files.newInputStream(myfile.toPath()); OutputStream os = java.nio.file.Files.newOutputStream(myfile.toPath());

This method uses the instanceof operator in a series of if/else statements to differentiate blocks of code based on type. If these types are related by inheritance, it is cleaner to just define a method in the base class, and use overridden methods in these classes.

This class is using singleton design pattern and has non-private constructor (please note that a default constructor might exist which is not private). Given that, it is possible to create a copy of the object, thus violating the singleton pattern.
The easier solution would be making the constructor private.

For more information, see: SEI CERT MSC07-J, and CWE-543: Use of Singleton Pattern Without Synchronization in a Multithreaded Context.

Found a call to a method which will perform a byte to String (or String to byte) conversion, and will assume that the default platform encoding is suitable. This will cause the application behavior to vary between platforms. Use an alternative API and specify a charset name or Charset object explicitly.

See CWE-173: Improper Handling of Alternate Encoding.

Cryptographic keys should not be kept in the source code. The source code can be widely shared in an enterprise environment, and is certainly shared in open source. To be managed safely, passwords and secret keys should be stored in separate configuration files or keystores. (Hard coded passwords are reported separately by the Hard coded password pattern)

Vulnerable Code:

byte[] key = {1, 2, 3, 4, 5, 6, 7, 8};
SecretKeySpec spec = new SecretKeySpec(key, "AES");
Cipher aes = Cipher.getInstance("AES");
aes.init(Cipher.ENCRYPT_MODE, spec);
return aesCipher.doFinal(secretData);

References
CWE-321: Use of Hard-coded Cryptographic Key

There is a statement or branch on an exception path that if executed guarantees that a value is null at this point, and that value that is guaranteed to be dereferenced (except on forward paths involving runtime exceptions).

This method uses a hand-typed String literal to specify a Charset encoding. As this class is compiled with JDK 7 (or better), and the charset in question is available as a constant from the java.nio.charset.StandardCharsets class, it is better to use the .name() method of the appropriate StandardCharsets constant.

The method in question doesn't directly support a Charset as a parameter, only a String. Still, instead of specifying something like "UTF-8" (and potentially mistyping it), use StandardCharsets.UTF_8.name().

This method stores the return result in a local variable, and then immediately returns the local variable. It would be simpler just to return the value that is assigned to the local variable, directly.

Instead of the following:

public float average(int[] arr) {
    float sum = 0;
    for (int i = 0; i < arr.length; i++) {
        sum += arr[i];
    }
    float ave = sum / arr.length;
    return ave;
}

public float average(int[] arr) {
    float sum = 0;
    for (int i = 0; i < arr.length; i++) {
        sum += arr[i];
    }
    return sum / arr.length; //Change
}