int search( int arr[], int size, int num) {

int i, loc;
loc=i;

for (i=0;i<size;i  ){
    if (arr[i]==num)
          return loc;
    else 
          return -1;
    }
} 

can someone tell me where did I go wrong?

CodePudding user response：

There are several problems with the shown code as described below.

Problem 1

The variables i and loc are uninitialized and using those uninitialized variables(which you did when you wrote loc = i) is undefined behavior.

Problem 2

The logic that you've used for the for loop and also for the if else part is not correctly implemented. In particular, suppose the for loop is not entered due to the condition i<size being false, then no return statement will be encountered for your non-void returning function again leading to undefined behavior.

Solution

There is no need to create a variable named loc. You can just directly return i when the element is found. Moreover there is no need for the else block. If the element is not found while iterating through the elements, we just return -1 after the for loop.

int search( int arr[], int size, int num) {
    for (int i=0; i < size; i  )
    {
        if (arr[i]==num)
              return i;//value found so return i directly 
    }
    return -1; //value was never found 
} 
int main()
{
    int arr[] = {1,2,33,54,3};
    std::cout<<search(arr, 5, 3)<<std::endl;  //prints 4 
    std::cout<<search(arr, 5, 33)<<std::endl; //prints 2 
    std::cout<<search(arr, 5, 98)<<std::endl; //prints -1
 
}

Working demo

CodePudding user response：

Here is a solution that resolves the issues in your initial code. Let's break it down by commenting the code.

int search( int arr[], int size, int num) {

    // Initialize `location` to a value, this is very important
    // In this case, it's okay to initialize to the "not found"
    // value and you can simply `return location` at the end of
    // this function.
    int location = -1;

    for (int index = 0; index < size; index   ) {

        // Check to see if `arr[index] == num`
        // If so, we found `num` in `arr` !
        // Otherwise, let the loop continue
        if (arr[index] == num) {

             // Since we found `num` in `arr`, let's store the index
             // by updating our location
             location = index;

             // We found the index we are looking for
             // No need to continue the `for` loop
             break;
        }
        // No need for an `else` here, as it was noted in comments that
        // returning in the `else` block  would exit your loop early
    }

    // Just return `location`. It either stores `-1` of the index of `num`
    return location;
} 

Take time to review your solution and compare it to this particular solution. The primary issues with your original code were

• You didn't initialize the loc variable nor did you ever update it before returning. This results in Undefined Behavior.
• You had an else statement that returned -1 inside your for loop, which resulted in the loop only ever iterating a single time

Note that there are several ways to write this function and I've just shown one approach. Consider how you could write something functionally equivalent using your own approach.

Also note that you could simplify this code to not use a location variable altogether, as in the answer provided by @AnoopRana. I've left the use of the location variable in my example since it seemed to be closer to what you were originally going for. This solution is verbose in an attempt to help you understand more thoroughly what the algorithm is doing.