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

Find all instances of element in array

Submitted by: @import:stackexchange-codereview··
0
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 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 Correctness

Since 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.