snippetcppMinor
Selection Sort in C++
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
I have looked into other solutions, but they didn't clear up much for me.
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
-
-
The outside
So it would be:
-
You actually can start on
-
The naming of the vector is not good, as one-letter names are hard to read. Change it to
Now all you need is the
Optionally, you can remove the
Also, you can extract the swapping part into a method. In fact, you should. This makes code (even this size) easier to maintain.
-
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.