0

I'm coding a simple local search algorithms for TSP in java. Here's the method:

public Permutation localSearch(Permutation best, int maxnoimprov) {
    int count = 0 ;
    Permutation candidate;
    do {
        candidate = stochastic_2_opt(best);
        count = (candidate.getLength() < best.getLength()) ? 0 : count+1;
        if (candidate.getLength() < best.getLength()) {
            best=candidate;
        }
        System.out.print("Candidate "); candidate.showPermutation(); System.out.println(" Current best: "+best.getLength());
    } while (count<maxnoimprov);
    return best;
}

The problem is that the if-statement is always true,so when running the method the output looks like this:

....3, 34, 43, 32, }LENGTH: 30464.0 Current best: 30464.0

....14, 37, 24, 49, }LENGTH: 31499.0 Current best: 31499.0

....8, 4, 20, 42, }LENGTH: 30710.0 Current best: 30710.0

....23, 33, 12, 6, }LENGTH: 29321.0 Current best: 29321.0

....11, 32, 28, 15, }LENGTH: 30545.0 Current best: 30545.0 ..........................................................

As you can see the "best" is always replaced by the "candidate" while it should not.

My code seems fine to me but something is obviously wrong.

NOTES:

1) I've checked the stochastic_2_opt() method and it's ok.

2) The getLength() method returns double values so i thought that could be a trap and i used Double.compare but even then didn't work.

3) I also notice that when writing the if-condition as (candidate.getLength() < best.getLength()) it's also always true.

Can you help me find where the mistake is?

ρss
  • 5,115
  • 8
  • 43
  • 73

1 Answers1

1

I suspect the Permutation member variable that's holding the length is a static variable; that could explain the results perfectly. Then when stochastic_2_opt() creates a candidate, it inadvertently sets the length of best, too. Your belief that the if is always true is a red herring, in that case; the if would evaluate to false, but you'd still get the same result.

Ernest Friedman-Hill
  • 80,601
  • 10
  • 150
  • 186
  • Inside the stochastic_2_opt() method the Permutation variable "length" is re-computed, before the Permutation is returned –  Aug 28 '11 at 19:15
  • So apparently you didn't realize that assigning `copy = p` doesn't actually copy `p`. It copies a *reference* to `p`. Then this method proceeds to change the innards of the object `p` points to. In other words, this method changes `best`'s contents. So it's not the declaration of the `length` variable -- it's just rather a complete misunderstanding of how Java works. – Ernest Friedman-Hill Aug 28 '11 at 19:21
  • You're right. It was a silly mistake i did while trying different things. However, even without the "copy" variable, the error remains. –  Aug 28 '11 at 19:31
  • I am not sure you're understanding this, still. Unless you, at some point, use `new Permutation()` to create a new `Permutation` object, you'll continue to have the same problem -- and if that member variable is `static` (you still haven't confirmed otherwise) then even then, you'd *still* have the same problem. – Ernest Friedman-Hill Aug 28 '11 at 19:44
  • I fixed it. i created a new constructor in the Permutation class just to replace a permutation with another so i added these lines: Permutation candidate = new Permutation(best); candidate.stochastic_2_opt(); Thank you for your help! * Just for the record the "length" variable is not static. –  Aug 28 '11 at 22:06