HiveBrain v1.2.0
Get Started
← Back to all entries
snippetcppMinor

Selection Sort in C++

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
sortselectionstackoverflow

Problem

I wrote a little C++ sorting routine for my Algorithm studies, but I feel that a lot can be skipped out.

I am aware that modern sort takes Iterators and comparator functions, let's disregard that for now. I just want to clean up this code. Specifically, I don't like the if and value variables.

I have looked into other solutions, but they didn't clear up much for me.

void SelectionSorter::sort(std::vector &v){

    int currentIndex = 0;

    while (currentIndex < v.size()) {

        int indexOfSmallestItem = -1;
        int value = v[currentIndex];
        bool shouldSwap = false;

        for (int i = currentIndex; i < v.size(); i++) {
            if (v[i] < value) {
                value = v[i];
                indexOfSmallestItem = i;
                shouldSwap = true;
            }
        }

        if (shouldSwap) {
            int temp = v[currentIndex];
            v[currentIndex] = v[indexOfSmallestItem];
            v[indexOfSmallestItem] = temp;
        }

        currentIndex++;
    }
}

Solution

Some points:

-
The shouldSwap variable is not needed. Instead, set indexOfSmallestItem to currentIndex at the beginning of the loop (this is necessary for the next step to work) and check is the indexOfSmallestItem is currentIndex:

if (indexOfSmallestItem != currentIndex) {
    int temp = v[currentIndex];
    v[currentIndex] = v[indexOfSmallestItem];
    v[indexOfSmallestItem] = temp;
}


-
value is also not needed, as you can access the vector directly:

if (v[i] < v[indexOfSmallestItem]) {
        value = v[i];
        indexOfSmallestItem = i;
    }


-
The outside while loop can be a for loop, because:

int currentIndex = 0; // initialization

while (currentIndex < v.size()) { // condition

    // ...

    currentIndex++; // increment
}


So it would be:

for (int currentIndex = 0; currentIndex < v.size(); currentIndex++) {
    // ...
}


-
You actually can start on currentIndex + 1 because the first one is the current value.

-
The naming of the vector is not good, as one-letter names are hard to read. Change it to vect or something similar.

Now all you need is the indexOfSmallestItem and the vector:

void SelectionSorter::sort(std::vector &vect) {

    for (int currentIndex = 0; currentIndex < vect.size(); currentIndex++) {

        int indexOfSmallestItem = currentIndex;

        for (int i = currentIndex + 1; i < vect.size(); i++) {
            if (vect[i] < vect[indexOfSmallestItem]) {
                indexOfSmallestItem = i;
            }
        }

        if (indexOfSmallestItem != currentIndex) {
            int temp = vect[currentIndex];
            vect[currentIndex] = vect[indexOfSmallestItem];
            vect[indexOfSmallestItem] = temp;
        }

    }

}


Optionally, you can remove the if (indexOfSmallestItem != currentIndex) completely, as it is not much of an improvement.

Also, you can extract the swapping part into a method. In fact, you should. This makes code (even this size) easier to maintain.

void SelectionSorter::sort(std::vector &vect) {
    for (int currentIndex = 0; currentIndex  &vect, int i, int j) {
    int temp = vect[i];
    vect[i] = vect[j];
    vect[j] = temp;
}

Code Snippets

if (indexOfSmallestItem != currentIndex) {
    int temp = v[currentIndex];
    v[currentIndex] = v[indexOfSmallestItem];
    v[indexOfSmallestItem] = temp;
}
if (v[i] < v[indexOfSmallestItem]) {
        value = v[i];
        indexOfSmallestItem = i;
    }
int currentIndex = 0; // initialization

while (currentIndex < v.size()) { // condition

    // ...

    currentIndex++; // increment
}
for (int currentIndex = 0; currentIndex < v.size(); currentIndex++) {
    // ...
}
void SelectionSorter::sort(std::vector<int> &vect) {

    for (int currentIndex = 0; currentIndex < vect.size(); currentIndex++) {

        int indexOfSmallestItem = currentIndex;

        for (int i = currentIndex + 1; i < vect.size(); i++) {
            if (vect[i] < vect[indexOfSmallestItem]) {
                indexOfSmallestItem = i;
            }
        }

        if (indexOfSmallestItem != currentIndex) {
            int temp = vect[currentIndex];
            vect[currentIndex] = vect[indexOfSmallestItem];
            vect[indexOfSmallestItem] = temp;
        }

    }

}

Context

StackExchange Code Review Q#112156, answer score: 5

Revisions (0)

No revisions yet.