patterncppModerate
Find all instances of element in array
Viewed 0 times
instancesallarrayelementfind
Problem
I've created a function to return all the indices where an element is to be found in an array. If it occurs twice or more, all the respective indices will be returned, for now it returns them in natural counting (1,2,3...) and not how the computer sees them (0,1,2,...).
#include
#include
using namespace std;
void searchList(int theArray[], int sizeOfTheArray, int findFor);
void searchList(int theArray[], int sizeOfTheArray, int findFor){
vector foundIndices;
int j = 0;
for (int i = 0; i < sizeOfTheArray; i++)
{
if (theArray[i] == findFor){
foundIndices.push_back(i);
j++;
}
}
if (foundIndices.size()!=0){
cout << "Found in index: ";
for (int i = 0; i < foundIndices.size(); i++){
cout << foundIndices[i]+1 << " ";
}
}
else
cout << "Not found in array";
}
int main(){
int test[8] = {87, 75, 98, 100, 100, 234, 265, 9};
searchList(test, 8, 99);
return 0;
}Solution
Separation of Concerns
The function should have only one concern--finding the indices. Printing out results is a separate concern that shouldn't be combined with finding the indices. Combining the two makes it difficult or impossible to use in a wide variety of situations.
Vacuous Declaration
Having the declaration followed immediately by the definition renders the declaration just extra visual noise.
Pointless Work
In your function, you initialize and increment
Inconsistent Types
The type normally used for indices into arrays is
Intermixing Arrays and Vectors
Although it's not exactly an error, it is a bit strange to have an array as the input type, and a vector as the result type. Typically you'd want to pick one or the other, and use it consistently. The other obvious possibility would be to have the function itself work with iterators, so it doesn't have to specify the type of the collection itself at all (and can be written generically, so it can work with essentially any collection).
Since this function shouldn't modify the input array, it's better to pass that as a pointer to
The function should have only one concern--finding the indices. Printing out results is a separate concern that shouldn't be combined with finding the indices. Combining the two makes it difficult or impossible to use in a wide variety of situations.
Vacuous Declaration
Having the declaration followed immediately by the definition renders the declaration just extra visual noise.
Pointless Work
In your function, you initialize and increment
j, but you never seem to make any use of it.Inconsistent Types
The type normally used for indices into arrays is
size_t, but you're returning a vector of int instead.Intermixing Arrays and Vectors
Although it's not exactly an error, it is a bit strange to have an array as the input type, and a vector as the result type. Typically you'd want to pick one or the other, and use it consistently. The other obvious possibility would be to have the function itself work with iterators, so it doesn't have to specify the type of the collection itself at all (and can be written generically, so it can work with essentially any collection).
const CorrectnessSince this function shouldn't modify the input array, it's better to pass that as a pointer to
const (note: although your parameter definition uses array notation, what's passed is actually a pointer). If you use a vector as input, you'd probably want to pass it as a reference to const.Context
StackExchange Code Review Q#124085, answer score: 13
Revisions (0)
No revisions yet.