Fix Issue #13 - It is possible !outputQueue.Empty() should be used instead of outputQueue.size()

[Echelon9]

Following further discussion on this Issue the desired fix is to refactor to the equivalent, slightly more elegant form

for (col=0; !outputQueue.Empty(); col++)
    path.Insert(outputQueue.Pop(), _FILE_AND_LINE_);

.. given for()'s second argument is expecting an expression which is contextually convertible to bool and that using .Size() there is an implicit conversion between size_t and bool (0 = false, 1+ = true)

[ViteFalcon]

I believe the intention is for the for-loop to continue as long as there are elements in X. NOTE that X.pop() is called, which, from initial looks, would deplete the number of elements in X for each iteration.

[Echelon9]

Perhaps rephrasing, isn't it always the case that a for loop through an STL container should continue as long as there are elements in X?

[ViteFalcon]

Not if you're doing it wrong.

With your change, consider size of container to be 5. Your loop variable, i, starts at 0 and gets incremented every iteration.

First iteration:
X.Size() = 5;
i = 0;

Second:
X.Size() = 4;
i=1;

And so on...

Will the container elements be emptied if the condition is i < X.Size()?

[Echelon9]

So it's equivalent to the slightly more elegant (IMHO)

for (col=0; !outputQueue.Empty(); col++)
    path.Insert(outputQueue.Pop(), _FILE_AND_LINE_);

.. given for()'s second argument is expecting an expression which is contextually convertible to bool and that using .Size() there is an implicit conversion between size_t and bool (0 = false, 1+ = true)

[ViteFalcon]

This definitely is a better solution that what's there already. But it isn't now in sync with the issue that this is intended to fix. The issue itself would be a no fix at its current state. You'll have to update the issue first and then change your current pull request to match your above solution.

[Echelon9]

I'll amend in the next few days after some travel. Thanks for discussing it.

[Echelon9]

Revised approach adopted. PR is ready.

[FastFrench]

As col variable is not used, why do you not simply write :
while (!outputQueue.Empty())
path.Insert(outputQueue.Pop(), FILE_AND_LINE);

[larku]

Agreed with FastFrench, solution should be:

while (!outputQueue.Empty())
  path.Insert(outputQueue.Pop(), _FILE_AND_LINE_);

This also expresses the intention more clearly.

[Echelon9]

Rebased and refactored based on discussion in this thread. Ready for merge.
Thanks for the contributions.

[larku]

Actually, while looking at this in a greater context, the col variable is used in the enclosing while condition:

while (costMatrix[row*adjacencyLists.Size() + col] == currentWeight)

At first glance this appears to stop the search at a condition that may never be met with the suggested optimisation.

Without further rationalisation of why that condition would be left out, I'd vote against this change for now.

[larku]

Ha, in fact your original suggestion:

for (col=0; !outputQueue.Empty(); col++)
    path.Insert(outputQueue.Pop(), _FILE_AND_LINE_);

Is, IMO the correct solution to maintain the current behaviour and also use a possibly more optimal and correct condition.

[Echelon9]

It seems a bit academic, given the project maintainers don't appear to be accepting PRs.

[Luke1410]

Pull request integrated in https://github.com/SLikeSoft/SLikeNet (incorporated in SLikeNet 0.1.0) - slightly modified by calling IsEmpty() instead of Empty().