CODEWITHSUNDEEP
java
Waz
Waz

Reputation: 161

Reduce complexity of an if statement containing lots of if statements

I have a method for which I need to reduce the cyclomatic complexity. But am unsure how to go about it. 

public boolean isEqual(Car car) {
    if (this.id != car.id) return false;
    if (this.reg != car.reg) return false;
    if (this.vin != car.vin) return false;
    if (this.make != car.make) return false;
    if (this.model != car.model) return false;

    return true;
}

There are more if statements like that in the method but I have only included a few. How can I reduce the complexity? Sorry if not correctly formatted as I have written this on my mobile.

Upvotes: 0

Views: 218

Answers (4)

Alan Hay
Alan Hay

Reputation: 23246

You could use a 3rd party library such as Apache Commons lang which has utility methods like:

public boolean isEqual(Car car) {
       return new EqualsBuilder().reflectionEquals(this, car);
    }

http://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/builder/EqualsBuilder.html

However ask if you really need to compare all fields: CarA and CarB can surely only be equal if they have the same id (or vin or registration).

public boolean isEqual(Car car) {
       //return this.reg= other.reg;?
       //return this.vin= other.vin;?
       return this.id = other.id;
}

Upvotes: 1

user949300
user949300

Reputation: 15729

Some would write it this way, a bit clearer (@krillgar suggested this too)

public boolean isEqual(Car car) {
   return (this.id == car.id) &&
          (this.reg == car.reg) &&
   ...
}

However, if even this seems too long, you should split it up into smaller tests for "sameness".

public boolean isEqual(Car car) {
   return this.sameMakeAndModel(car) &&
          this.sameRegistrationInfo(car) &&
   ...
}


boolean sameMakeAndModel(Car car) {
   return (this.make == car.make) &&
          (this.model == car.model) &&
          ...    check engine size, colors?
}

boolean sameRegistrationInfo(Car car) {
   return (this.vin == car.vin) &&
          (this.reg == car.reg) &&
          ...    
}

Whether these sub-tests should be public, protected or private is TBD.

p.s. Out of paranoia, add a test that the car argument is non-null!

Upvotes: 0

krillgar
krillgar

Reputation: 12815

There really is no way to make it that much cleaner. As Maroun questioned in the comments, there isn't really that much complexity. However, you can get rid of the "if" statements, and it would be a little easier to read this way:

public boolean isEqual(Car car) {
    return this.Id == car.id &&
           this.reg == car.reg &&
           this.vin == car.vin &&
           this.make == car.make &&
           this.model == car.model;
}

You can continue to add as many as you need there. This will act somewhat like your "if" statements, as the second it finds a "false" result, it will stop checking any others, and just return the false result.

Upvotes: 1

Ricardo Silva
Ricardo Silva

Reputation: 1384

you can do:

 public boolean isEqual(Car car) {
 return 
    !(this.id != car.id
    || this.reg != car.reg
    || this.vin != car.vin
    || this.make != car.make
    || this.model != car.model);
 }

Upvotes: 0

Related Questions