C++ function to find current number in vector

Question

I need explanation to my homework :/ I need to write a function in C++ which takes 3 arg ( initial and final iterator of a vector slice and int) and return const iterator of first found int(3th arg) in this vector. If this int is not in our vector, we should return const final vector iterator. This function use iterators instead of indexes and use only "vector header". This is example program

int main() {
const std::vector<int> vector {3, -1, 7, 12, -5, 7, 10};
std::cout << find(vector.begin() + 3, vector.end(), 7) - vector.begin() << std::endl; }

and this is my attempt to solve

#include <iostream>
#include <vector>
using namespace std;
vector<int>::const_iterator find(vector<int>::const_iterator, vector<int>::const_iterator, int);

int main() {

    const std::vector<int> vector {3, -1, 7, 12, -5, 7, 10};
    std::cout << find(vector.begin() + 3, vector.end(), 7) - vector.begin() << std::endl;

}


vector<int>::const_iterator find(vector<int>::const_iterator begin, vector<int>::const_iterator end, int num) {

        for(vector<int>::iterator iterator = begin; iterator != end;) {
            if (*iterator == num){
                return iterator;
            } else {
                return end;
            }
        }
}

What's the problem? Looks like a good start to me. Don't forget to actually increment the iterator! — Lightness Races in Orbit, Nov 23 '18 at 17:28
And why do you [attempt to] switch from `const_iterator` to `iterator` halfway through? — Lightness Races in Orbit, Nov 23 '18 at 17:29
for(vector::iterator iterator = begin; iterator != end;) { if (*iterator == num){ return iterator; } return end; } — Matthieu Brucher, Nov 23 '18 at 17:31
Reconsider that `else`. What the code currently says is if I don't find the number in the first try, I give up. — user4581301, Nov 23 '18 at 17:39
yea Stephan ... sry :) I forgot to ask. I wanted to say that it's not working and yes you are right Jarod, I need to ++ that. And also Races in Orbit yeaaa that is also mistake. And thank you Matthieu for help ! Sorry guys ..... I read books, watch guides and a trying to code c++ ( because we use it at school), but I just feel it's not my world :/ When Racec wrote "Looks like a good start to me" I was shocked, because every time when I write next line I'm like - "why I'm doing it like that ... it's all bad and ugly code and I have no idea what to do with this syntax". :/ — System, Nov 23 '18 at 17:40
Two-liner: `for (; begin != end && *begin != num; ++begin); return begin;` — Swordfish, Nov 23 '18 at 17:44
I like to visualize the problem. I draw lots of pictures to make sure the program I have in mind does everything in the right order before I start writing much in the way of code. [Flow diagrams](https://en.wikipedia.org/wiki/Flow_diagram) seem to have gone out of vogue, but I still like them. If I can walk through the flow diagram by pointing at a spot, write down the state of the variables involved, and moving on to the next spot in the diagram and it all makes sense, the code is probably going to work. — user4581301, Nov 23 '18 at 17:48

user4581301 · Accepted Answer · 2018-11-23T20:38:44.970

Breaking this down, I see one compiler error and one warning (a logical error) and one logical error not exposed by the compiler. First let's deal with the error first.

In

for(vector<int>::iterator iterator = begin; iterator != end;)

the

vector<int>::iterator iterator = begin

tries to make a vector<int>::iterator out of a vector<int>::const_iterator. The easy fix is

for(vector<int>::const_iterator iterator = begin; iterator != end;)

But begin has been passed into the function by value. This means you can do whatever you want to it inside the function without affecting it outside the function. This means making the variable iterator isn't necessary at all.

for(/*do nothing here*/; begin != end;)

is sufficient to eliminate the error. This leads to the logical error.

for(vector<int>::iterator iterator = begin; iterator != end;)

and the improved version

for(/*do nothing here*/; begin != end;)

do nothing to advance the iterator to make the loop look at more than the first value.

for(/*do nothing here*/; begin != end; ++begin)

takes care of that.

This leaves us with just the compiler warning. Never ignore warnings. Programmers tend to be lazy and obsessed with speed. They wouldn't go to the effort and incur the performance penalty of printing out a message if it didn't mean something important. A compiler error means the syntax is wrong and the code cannot be turned into a program. A compiler warning means the syntax is correct and, in the absence of other errors, the code can be turned into a program, but the program probably isn't logically correct. Never ignore warnings. Eliminate them where possible. If they can't be eliminated, prove you get the behaviour you require and justify them. Just don't ignore them.

The warning should be something like function find does not return a value on all paths.

Note: I have adjusted the code to the Allman indentation style. I find that its placement of braces and extremely regular flow makes misplaced or missing brackets and scoping errors easy to spot. If you are having problems with syntax, this style eliminates possible sources of confusion and lets you focus on other problems.

vector<int>::const_iterator find(vector<int>::const_iterator begin, 
                                 vector<int>::const_iterator end, 
                                 int num) 
{
    for(/*do nothing here*/; begin != end; ++begin)
    {
        if (*begin== num)
        {
            return begin;
        } 
        else 
        {
            return end;
        }
    }
}

Both return statements are inside the for loop. If begin == end at the start, the for loop will never enter and the function will exit without a valid return. The results of exiting a function that is declared to return a value without returning a value is undefined. The program could do anything, including look like it worked.

What this really points out is return end; isn't where you want it to be. It should be placed after all of the values have been inspected and found wanting. In other words, outside the loop. This makes the else case empty and useless. Remove it.

That leaves

vector<int>::const_iterator find(vector<int>::const_iterator begin, 
                                 vector<int>::const_iterator end, 
                                 int num) 
{
    for(/*do nothing here*/; begin != end; ++begin)
    {
        if (*begin == num)
        {
            return begin;
        } 
    }
    return end;
}

which can be improved upon for brevity, but I believe results in the most easily understood code. Short code is nice, but easy-to-read code that the compiler will transform into the same program as the shorter code is more valuable in my eyes.

Sidenote:

It is risky to give a variable the same name as its type.

vector<int>::iterator iterator

is safe because the the name of the type is vector<int>::iterator, not merely iterator, but a common early bug looks something like

string string; 
string name; // inscrutable error message here.

string, the variable of type string, takes the name string making string the type inaccessible when string is used again by string name. The poor compiler thinks it's been told to make a variable of a type that is another variable. This is also a great example of Why is "using namespace std" considered bad practice? std::string string; would make the whole problem impossible, at least for types in the std namespace.

.... it's so so hard to find good teacher and your answer is great, because now I understand more ! THANK YOU :) — System, Nov 23 '18 at 19:40

C++ function to find current number in vector

1 Answers1