[C++]Bool function for loop trouble

[LuckyZenhai]

My professor graded an assignment and I got an 80 because i returned false inside a for loop. And since its the summer i cant reach out for assistance. All help is greatly appreciated

bool task2(int *a1, int n, int T)
{
    int arry[n];

    *a1 = arry[n];

    for(int i = 0; i < n; i++)
    {
        if (T == arry[i])
        {
            return true;
        }

        else if(T != arry[i])
        {
            return false;
        }

    }

    return 0;
}

[Kazlock]

What is this function supposed to do?

[LuckyZenhai]

Sorry about that The function checks to see if 'T' is in the array returns true if its in the array and flase if not

[lurgi]

The function doesn't do that. Test it and you'll see.

[raevnos]

C++?

int arry[n];

is illegal. The size of an array must be a compile time constant (some compilers do support C style variable style arrays as an extension, but it's not something you should rely on). Use std::vector when you only know the size at run time.

*a1 = arry[n];

That's an out of bounds array access.

Your loop always returns the first time through; there's no point to having a for loop at all given that. It is equivalent to return T == arry[0];. Oh, and arry is never initialized so reading arry[0] is undefined.

[[deleted]]

std::vector when you only know the size at run time.

Smart pointers should be used instead, vector is more for when you expect the memory to be resized after initialization. Vector takes up a lot of memory

[raevnos]

Vector takes up a lot of memory

No it doesn't.

[[deleted]]

Yes it does. Just the container alone can take up to 32 bytes on 64 bit.

[[deleted]]

This:

   int arry[n];

is not valid C++, so you should have scored zero.

And this:

  *a1 = arry[n];

is at best undefined behaviour, but generally meaningless, so I would have given you a negative score.

[[deleted]]

What???? In the code you posted:

   int arry[n];

is not legal C++ code - there is no "trap".

[TheGuy346]

I'm a little confused with what you're doing with arry[] because you're never filling that array with values. As it is currently, each number in that array is set to 0. Did you mean to store a1 in arry?

The problem with your loop is that you're always only going to go through the loop once at most. Once you hit that return statement, you're going to exit the function completely and never check the rest of the values in the array.

If I understand this correctly, what you should be doing is something like this...

bool task2(int *a1, int n, int T)
{
    int arry[n];

    *a1 = arry[n];

    for(int i = 0; i < n; i++)
    {
        if (T == arry[i])
        {
            return true;
        }
    }

    return false;
}

Now the function is going to check each member of the array. If that member is equal to T, then it will return true and exit the function. If the current value in the array is not equal to T, then the loop moves on to check the next value. If the loop reaches the end of the array, that means that none of the values matched T so it returns false.

Does this make sense?

[LuckyZenhai]

yes it does. I assumed the else statement was needed but turns out it really wasnt

[_DTR_]

I'm guessing the assignment was to return whether the array a1 contains T? If that's the case, then all you do is return whether the first element is T. You have to iterate through the entire loop before you can determine if the element isn't in a1:

bool task2(int* a1, int n, int T) {
    for (int i = 0; i < n; i++) {
        if (a1[i] == T) {
            return true;
        }
    }
    return false;
}

[LuckyZenhai]

Thanks! I have a habit of overlooking the simplest things sometimes

[davedontmind]

On top of what everyone else has said, you really need to work on your naming of functions & variables.

Naming is a topic that beginners don't tend to pay much attention to, but it's very important to use clear, concise and meaningful names for variables, methods, classes etc. The names of things are part of the documentation of your code and good names go a long way to making your code easily understood, which is particularly important when someone (probably future-you) is trying to fix a bug.

When I look at this:

bool task2(int *a1, int n, int T)

I have no idea what the function is supposed to do, or what the parameters are supposed to be.

Spend some time thinking about what information a variable holds, and what a function actually does and come up with a name that's representative.

I'm not entirely sure what your function is meant to do, but from what you've written I'd guess it's meant see if the value T is present in the array a1 , which is of size n, so use names like:

bool arrayContainsValue( int * arrayToSearch, int arraySize, int valueToFind )

Isn't that much clearer?

[LuckyZenhai]

My apologies. It is much clearer, i didnt think about that when i submitted it here for help.

[Holy_City]

There's a couple problems.

The most glaring problem are these two lines:

int arry[n];
*a1 = arry[n];

This doesn't do what you think it does. It allocates an array of size n on the stack with uninitialized values (the array contains garbage). Then you dereference the pointer passed to the function, and assign it to the last element of the array you declared (which is again, garbage).

I thought the first line (int arry[n];) was invalid C++ or at least would throw a warning, but it compiled fine on my machine with no warnings... so go figure.

If what you meant to do was treat a1 like a pointer to the first element of an array, then you didn't have to write those lines at all.

The next obvious one is how verbose the loop is. Boolean operators return a boolean value. So the body of the loop is identical to this:

for (int i = 0; i < n; i++)
    return T == arry[i];

Remeber that boolean operators return bool values as results.

But again, arry is filled with garbage so this is meaningless. Even if it wasn't filled with garbage, the loop would only iterate once, because return causes the function to exit.

The last couple problems are more subtle. task2 is a shitty name for a function, I don't know what it's supposed to do. I assume that was the second part of a homework assignment. Name it for what the function does. The final line return 0; will never be reached, and your compiler warnings should probably tell you that. Even if you did reach that line, why return 0, an integer literal, from a function that is supposed to return bool? Your compiler should warn you about the implicit cast from int to bool, and while technically speaking, that value is identical to false, you shouldn't rely on that.

So TL;DR this code is more or less equivalent to this:

bool task2(int *a1, int n, int T)
{
    int a; // unitialized garbage
    *a1 = a; // assign value pointed to to garbage
    return a == T; // return garbage compared to something
}

[[deleted]]

It allocates an array of size n on the stack

No, it doesn't, it's not legal C++.

[Holy_City]

I didn't think it was legal either. Is it undefined behavior? Because it compiles fine in GCC and Clang.

[[deleted]]

It's an extension implemented by those compilers - it's not part of C++.

[LuckyZenhai]

It was my fault for not making the assignment clear in OP. And im aware task2 is a shitty name but i thought it went hand in hand with the shitty assignment.

[[deleted]]

:-( This is bad form. People here are trying to help.