views:

159

answers:

3

I have some technical question, I have repeating code in my work, and I want to get rid of it, so I know that in C++ it is not good idea to use macro, but instead I must use inline function, is it good idea to use this function as inline:

list<Data>::iterator foo(int data){
if(dataExists(data)){
    list<Data>::iterator i;
    for(i = dataClass.begin(); i != dataClass.end(); ++i){
       if(i->getData() == data){
        break;
       }
return i;   //here I have one more problem, what can I return if data doesn't exist?
}

I'm beginner, and I think that this function is very unsafe, can somebody please give me advice, how can I improve my code, thanks in advance

P.S. what usually do You use to avoid repeating code?

I edited code

+8  A: 

What you're doing here is already done by the std::find() function, so it would be better to use that (although it's certainly OK to try implementing these things yourself for the exercise).

std::find() also demonstrates a good way to indicate the "not found" condition -- if the item is not found, it returns the iterator one-past-the-end. That way, the caller can determine whether a matching item was found by comparing the iterator returned with Data.end().

j_random_hacker
@j_random_hacker: list doesn't have find, am I wrong?
helloWorld
Nope, there is not `std::list<T>::find()`. But there is `std::find()` that you use like this: `std::find(myList.begin(), myList.end(), dataToFind)`. See http://www.cppreference.com/wiki/stl/algorithm/find
Ferdinand Beyer
One more thing: You'll have to `#include <algorithm>` to use `std::find()` and friends.
Ferdinand Beyer
+1, but in this particular case you will need to use `find_if` together with a predicate that tests the equality of the value returned by the member function and the value you are looking for...
David Rodríguez - dribeas
+4  A: 

The code you posted makes no sense. at one point you use Data as a type, and at another as an object. Assuming it is an object, the way to indicate that something is not found would be to return an iterator pointing to its end.

return Data.end();

but your code is too confused for this to work without major changes.

You are not going to learn C++ by posting questions on SO - it simply cannot be done. Instead, you need to read a good C++ text book - I recommend Accelerated C++, which covers things like iterators in detail. If you carry on askking questions like this, you are simply wasting your time and, more importantly, ours.

anon
Hear, hear.....
Skilldrick
+1  A: 

I don't really know what you are asking here, but there are several misconceptions in your post.

First inline functions are a specific optimization you do not need to care about quite yet. Learn about normal functions first and understand them properly.

As another answer already said, std::find() does what you seem to want to do. It doesn't have to be a member of list to work, in fact modern C++ styleguides often prefer non member functions.

Now to your code. I am pretty sure that the code you posted is not working C++ code, which makes it really hard to understand what you are trying to do. The list type you are using is also not a std::list<> (The iterators work differently), and you use Data as both a variable (which is not defined in your code) and as a type.

As someone already suggested maybe you should start a little easier, or better yet, get a good book (I recommend "Accelerated C++" and "Programming P&P using C++") for C++ beginners.

Fabio Fracassi