Bugs found by FindBugs plugin Eclipse

Question

using bug finder plugin, I found this bugs but does not understand why it was seen as bug in the code. Does anybody know and give me proper explanation regarding these? Thanks.

Source Code - https://drive.google.com/open?id=1gAyHFcdHBShV-9oC5G7GeOtCGf7bXoso;

Patient.java:17 Patient.generatePriority() uses the nextDouble method of Random to generate a random integer; using nextInt is more efficient [Of Concern(18), Normal confidence]

 public int generatePriority(){
    Random random = new Random();
    int n = 5;
    return (int)(random.nextDouble()*n);
 }

ExaminationRoom.java:25 ExaminationRoom defines equals and uses Object.hashCode() [Of Concern(16), Normal confidence]

public boolean equals(ExaminationRoom room){
        if (this.getWaitingPatients().size() == room.getWaitingPatients().size()){
            return true;
        }
        else {
            return false;
        }
    }

ExaminationRoom.java:15 ExaminationRoom defines compareTo(ExaminationRoom) and uses Object.equals() [Of Concern(16), Normal confidence]

    // Compares sizes of waiting lists
    @Override
    public int compareTo(ExaminationRoom o) {
        if (this.getWaitingPatients().size() > o.getWaitingPatients().size()){
            return 1;
        }
        else if (this.getWaitingPatients().size() < o.getWaitingPatients().size()){
            return -1;
        }
        return 0;
    }

Hospital.java:41 Bad month value of 12 passed to new java.util.GregorianCalendar(int, int, int) in Hospital.initializeHospital() [Scary(7), Normal confidence]

    doctors.add(new Doctor("Hermione", "Granger", new GregorianCalendar(1988, 12, 10), Specialty.PSY, room102));

Person.java:29 Return value of String.toLowerCase() ignored in Person.getFullName() [Scariest(3), High confidence]

public String getFullName(){
    firstName.toLowerCase();
    Character.toUpperCase(firstName.charAt(0));
    lastName.toLowerCase();
    Character.toUpperCase(lastName.charAt(0));
    return firstName + " " + lastName;

}

The months start at 0. So december is 11. You are passing 12. — marstran, Jan 26 '20 at 05:35
I recommend you don't use `GregorianCalendar`. That class is poorly designed and long outdated. One confusing thing about it and probably its most prominent design error is that it nimbers months 0 through 11. Instead use `LocalDate` from the modern java.time. Either `LocalDate.of(1988, Month.DECEMBER, 10)` or `LocalDate.of(1988, 12, 10)`. No confusion left. — Ole V.V., Jan 26 '20 at 06:22
By the way, your code can be simplified to improve readability: `public boolean equals(ExaminationRoom other){ return getWaitingPatients().size() == other.getWaitingPatients().size(); }` and `@Override public int compareTo(ExaminationRoom other) { return getWaitingPatients().size() - other.getWaitingPatients().size(); }`. — howlger, Jan 26 '20 at 09:41

Ole V.V. · Answer 1 · 2020-01-27T19:03:25.220

Don’t create a new Random object each time.
Use random.nextInt(n).
Define a hashCode method in ExaminationRoom.
Having your compareTo method be inconsistent with equals() may or may not be OK.
Use LocalDate instead of GregorianCalendar.
Pick up and use the return values from String.toLowerCase() and Character.toUpperCase().
Consider SpotBugs as a newer alternative to FindBugs.

Details

Random

Creating a new Random object each time you need one gives your poorer pseudo-random numbers with a high risk of numbers being repeated. Declare a static variable holding the Random object outside your method and initialize it in the declaration (Random is thread-safe, so you can safely do that). For drawing a pseudo-random number from 0 through 4 use

int n = 5;
return random.nextInt(n);

It’s not only more efficient (as FindBugs says), I first of all find it much more readable.

hashCode()

@Override
public int hashCode() {
    return Objects.hash(getWaitingPatients());
}

compareTo()

The equals method you have shown us seems to contradict FindBugs here. It does look a bit funny, though. Are two waiting rooms considered the same if they have the same number of waiting patients? Please think again. If you end up deciding that they are not equal but should be sorted into the same spot without discrimination, your compareTo method is inconsistent with equals(). If so, please insert a comment stating this fact. If you want FindBugs not to report this as a bug in subsequent analyses, you have two options:

Insert an annotation telling FindBugs to ignore the “bug”.
Create a FindBugs ignore XML file including this point.

I’m sorry I don’t remember the detauls of each, but your search engine should be helpful.

Don’t use GregorianCalendar

The GregorianCalendar class is poorly designed and long outdated. I suggest you evict it from your code and use LocalDate from java.time, the modern Java date and time API, instead.

doctors.add(new Doctor("Hermione", "Granger", LocalDate.of(1988, Month.DECEMBER, 10), Specialty.PSY, room102));

String.toLowerCase()

This was already treated in the other answer. Changing a name to have the first letter in upper case and the rest in lower case is not as simple as it sounds.

firstName.toLowerCase();
Character.toUpperCase(firstName.charAt(0));

The first of these two lines doesn’t modify the string firstName because strings have been designed to be immutable and toLowerCase() to return a new string with all letters in lower case (according to the rules of the default locale of the JVM, confusing). The second line also doesn’t modify any character because Java is call-by-value (look it up), so no method can modify a variable passed as argument. You’re not even passing a variable, but the return value from a different method. Also Character.toUpperCase() returns a new char in lower case.

What you need to do is pick up the values returned from these two method calls, use a substring operation for removing the first letter from the lower-case version of the name and concatenate the upper-case version of that letter with the remainder of the lower-case string. If it’s complicated, I am sure that your search engine can find examples of where and how it is done.

A bit of an aside: You may want to think twice before forcing Dr. Jack McNeil to be written as Mcneil and Dr. Ludwig von Saulsbourg as Von saulsbourg.

SpotBugs

It’s only something I have heard, I haven’t checked myself. The source code of FindBugs has been taken over by a project called SpotBugs. They say that SpotBugs is being developed more actively than FindBugs. So you may consider switching. I am myself a happy SpotBugs user in my daily work.

Links

What issues should be considered when overriding equals and hashCode in Java?
Documentation of Comparable explaining what it means that compareTo() is inconsistent with equals() and how to go about it.
Oracle tutorial: Date Time explaining how to use java.time.
Chicago Hope listing Jack McNeil as Orthopedic Surgeon.
Dr. Jack mentioning Dr. Ludwig von Saulsbourg on the cast.
SpotBugs

cameron1024 · Accepted Answer · 2020-01-26T07:24:29.647

First thing to remember about "bug finder" tools, is that they are usually only guidelines. With that said:

The class GregorianCalendar counts months from 0, meaning 0 is January, 11 is December. 12 represents the 13th month which doesn't exist. Since the function expects an int, and you gave it an int, no compiler error is generated, even though this is certainly a bug. This article does a good job explaining the reasons to upgrade, and give examples of how to use the new APIs: https://www.baeldung.com/java-8-date-time-intro

If in doubt, you can always check the documentation. In this case, the class Calendar (which GregorianCalendar extends) delcares a static constant public static final int JANUARY = 0; This confirms that january is indeed 0, but also indicates that we can use this constant in our code. You might find new GregorianCalendar(1988, Calendar.JANUARY, 10) to be a bit more readable.

You may also want to consider switching to the more modern and standard systems used to deal with time. The Java 8 Time libraries are the "new standard", and are definitely worth looking into.

Secondly, Strings are immutable in Java. This means that once a String is created, its value can never be changed. This may be counter to your intuitions, as you may have seen code such as:

String s = "hello";
s = s + " world";

However, this doesn't modify the string s. Instead, s + " world" creates a new String, and assigns it to the variable s.

Similarly, s.toLowerCase() doesn't change what s is, it only generates a new String which you must assign.

You probably want firstName = firstName.toLowerCase();

With your first example, nothing immediately jumps out to me as "bad", but if you look at the messages generated by your tool, they label the first example as "Of Concern", but label the others (e.g. the string.toLowerCase() example) as "Scary"/"Scariest". Although I am not familiar with this tool in particular, I imagine this is indicating more of a "Code Smell", rather than an actual bug.

Perhaps look into Unit Testing, if you want to reassure yourself that your code works.

Thank you . that gave me enlightment about the code. How about the first one? — Animoto Dito, Jan 26 '20 at 06:21
Agreed, my wording sounds too general. I'll edit my answer. I guess when in doubt just look at the documentation — cameron1024, Jan 26 '20 at 07:04

Bugs found by FindBugs plugin Eclipse

2 Answers2

Details

Links