I tried to find the location of the array's element, but the professor said it was wrong

Question

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?

Answer 1

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.

Answer 2

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.

Answer 3

See your question is not clear, but assuming you have three vectors of nodes of different types (int, float, std::string), and you want to return a value based on index. In that case you can use std::variant if you are using C++17 or above.

using MyVariant = std::variant<int,double,std::string>;
MyVariant list::at(int index)
{
    ...
}

For C++14 or below, you can look for union (better to wrap it in a class) or use void* (not recommended). Or use boost::variant for C++11 and C++14.

Answer 4

It's hard to give a solid answer to your question which lacks details. But I still want to suggest a potential solution. Keep in mind that this is only a suggestion.

You might return a std::tuple containing 3 std::optional values:

auto list::at(const int index)
{
    std::tuple< std::optional<int>, std::optional<double>,
                std::optional<std::string> > returnValue;

    for (auto node : VECTOR_OF_INT)
    {
        if (node.getIndex() == index)
        {
            std::get<0>(returnValue) = node.getValue();
            std::get<1>(returnValue) = { };
            std::get<2>(returnValue) = { };
            return returnValue;
        }
    }

    if ( !std::get<0>(returnValue).has_value( ) )
    {
        std::get<0>(returnValue) = { };
    }

    for (auto node : VECTOR_OF_DOUBLE)
    {
        if (node.getIndex() == index)
        {
            std::get<1>(returnValue) = node.getValue();
            std::get<2>(returnValue) = { };
            return returnValue;
        }
    }

    if ( !std::get<1>(returnValue).has_value( ) )
    {
        std::get<1>(returnValue) = { };
    }

    for (auto node : VECTOR_OF_STRING)
    {
        if (node.getIndex() == index)
        {
            std::get<2>(returnValue) = node.getValue();
            return returnValue;
        }
    }

    if ( !std::get<2>(returnValue).has_value( ) )
    {
        std::get<2>(returnValue) = { };
    }

    return returnValue;
}

But without knowing your circumstances, I can't say if this will be a nice way of doing it or not.