CODEWITHSUNDEEP
javaarraysobjectequals
Anas Z.
Anas Z.

Reputation: 37

Equals method not working

I am a new Java programmer , im trying to implement a method to check the equality between two "features" array inside my object "FeatureVector" seems really basic but the method is not working for some reason ; it doesnt produce logical results ,and i cant seem to find a solution, please help 

public boolean equals (FeatureVector x )
{
    boolean result =false ; 
    boolean size = false ;
    for (int i =0 ; (i < this.features.length && result == true );i ++  )
    {
        if (this.features[i] == x.features[i] ) {result = true ;}
        else {result = false ; }
    }

    if (this.features.length == x.features.length ) {size = true ;}
    else {size =false; }
    return (result && size) ; 
}

Upvotes: 0

Views: 641

Answers (3)

Adam Liss
Adam Liss

Reputation: 48330

The bug in the initial code was initializing result to false. That caused the loop to exit immediately, before the first comparison.

Note that it's considered a less-than-best practice to compare boolean values to true and false. At best, it's redundant. At worst, you're likely to create an error that's hard to spot:

if (some_value = false) {  // DON'T do this -- it's always false!

I've suggested before that, if you absolutely must do this, perhaps due to an undiagnosed psychological condition or a tech lead who should really have been in management, protect yourself by using Yoda conditions:

if (false == some_value) {  // Syntax error, a single "=" will create.

Here's a corrected and optimized version of the original code:

public boolean equals (FeatureVector x) {

  // Do the "cheapest" test first, so you have an opportunity to return
  // without waiting for the loop to run.
  if (this.features.length != x.features.length) {
     return false;
  }

  // There's no need to "accumulate" the results of each comparison
  // because  you can return immediately when a mismatch is detected.
  for (int i = 0; i < this.features.length; i++) {
    if (this.features[i] != x.features[i]) {
      return false;
    }
  }
  return true;
}

Upvotes: 6

lebolo
lebolo

Reputation: 2160

There are a few things that could go wrong with your logic. I'll rewrite it and comment as I go along.

public boolean equals (FeatureVector x )
{

    /*
     * Check for the length first, if the lengths don't match then
     * you don't have to bother checking each elements. Saves time!
     */
    if (this.features.length != x.features.length) return false;

    for (int i =0 ; i < this.features.length; i++) {
        /*
         * As soon as you find a mismatching element, return out of the
         * loop and method, no need to keep checking. Saves time!
         */
        if (this.features[i] != x.features[i]) return false;
    }

    // If the logic makes it this far, then all elements are equal
    return true;
}

Upvotes: -1

Sergey Kalinichenko
Sergey Kalinichenko

Reputation: 726987

You should switch the order of comparing the length and comparing the individual features: if the lengths are different, there's no point of comparing the rest!

You should also return false as soon as you know there's a difference - again, the only reason to continue with the loop is if you think that you may return true.

Here is how you can change your program:

public boolean equals (FeatureVector x )
{
    if (this.features.length != x.features.length ) {
        return false;
    }
    // If we get here, the sizes are the same:
    for (int i = 0 ; i < this.features.length ; i++)
    {
        if (this.features[i] != x.features[i] ) {
            return false;
        }
    }
    // If we got here, the sizes are the same, and all elements are also the same:
    return true; 
}

Upvotes: 1

Related Questions