Returning a pointer to a class within a class

Question

this is the first time I've done something like this so I'm a little uncertain how I need to do this. I have a very simple class which contains some simple values and some getters:

class Nucleotide{
    private:
        char Base;
        int Position;
        int Polymorphic;
    public:
        Nucleotide(char ch, int pos);
        int getPos();
        char getBase();
        int getPoly();
};

This class is present in another class that contains a vector of them:

class NucleotideSequence{
    private:
        std::string Name;
        std::vector<Nucleotide> Sequence;
    public:
        NucleotideSequence(std::string name, std::vector<Nucleotide> seq);
        std::string getName();
        Nucleotide getBase(int pos1);
};

I want the method of the second class called getBase to be able to take a integer - say 1, and return the first Nucleotide object in the vector. What I've written is below:

Nucleotide NucleotideSequence::getBase(int pos1)
{
    for(std::vector<Nucleotide>::iterator i = Sequence.begin(); i != Sequence.end(); i++)
    {
        if(pos1 == (*i).getPos())
        {
            return i; // Return a pointer to the correct base.
        }
    }
}

I've got Nucleotide as the return type but I was wondering really how I should change this - since if I return nucleotide because of pass by value would it not just return a copy of the object at that place in the vector? So I'd rather return a pointer/reference. I'm using an iterator in the loop so should I just return a pointer with the value of the iterator? How do I do this? In the function I return i but should I be returning i&? I'm uncertain about the specifics - presumably if I'm returning a pointer my return type needs to be Nucleotide* or perhaps Nucleotide& since & means address of? I've thought this through and read Cpp tuts but I'm still slightly unsure of the right answer.

Thanks, Ben.

Answer 1

Your code will benefit from some use of references for efficiency's sake. The getBase method signature should look like this:

const Nucleotide& NucleotideSequence::getBase(int pos1)

The NucleotideSequence constructor signature should look like this:

NucleotideSequence(const std::string& name, const std::vector<Nucleotide>& seq);

And the getName method like this:

const std::string& getName();

(Although return value optimisation might make that less important.)

As for the contents of getBase, it might help understanding to break down the code into:

const Nucleotide* NucleotideSequence::getBase(int pos1)
{
    for(std::vector<Nucleotide>::iterator i = Sequence.begin(); i != Sequence.end(); ++i)
    {
        Nucleotide& ref = *i; //Get a reference to the object this iterator points to
        if(pos1 == ref.getPos()) //compare its base to the argument
        {
            return &ref; // Return a pointer to the correct object.
        }
    }
    return NULL; //or null if we didn't find the object we wanted
}

Answer 2

As far as your question is concerned, returning a reference (&) as suggested by others is the solution.

In order to improve your code, I would as well suggest a change:

Either go for the operator[], or use the at() present in std::vector.

Thus, you can directly say:

return Sequence[pos1]; or return Sequence.at(pos1);

Answer 3

You have to return the Nucleotide by reference:

Nucleotide & NucleotideSequence::getBase(int pos1)
{
    for(std::vector<Nucleotide>::iterator i = Sequence.begin(); i != Sequence.end(); i++)
    {
        if(pos1 == (*i).getPos())
        {
            return *i; // Notice the *i instead of i
        }
    }
}

A reference works very similarly to pointer (allows you to pass the actual object, not its copy), but cannot be null and cannot point to non-existing object, so it's a lot safer than pointer.

Note though, that if you don't find the desired Nucleotide, you don't return anything, what generally is not a good idea. In this case using pointers may actually be a better idea:

Nucleotide * NucleotideSequence::getBase(int pos1)
{
    for(std::vector<Nucleotide>::iterator i = Sequence.begin(); i != Sequence.end(); i++)
    {
        if(pos1 == (*i).getPos())
        {
            return &(*i); 
        }
    }

    return nullptr;
}

Answer 4

You don't return a pointer, you attempt to return the iterator. And the function is declared to return an instance and not a pointer. Also, if you don't find the Nucleotide you don't return anything at all leading to undefined behavior if you try to use the "returned" value.

You could change the function to return a pointer, or a reference, or just a by value (copying like it's declared like not.

You can also change so that the function takes the Nucleotide as an argument instead, and then return a boolean indicator if it was found or not.

bool NucleotideSequence::getBase(int pos1, Nucleotide& n)
{
    for (...)
    {
        if (...)
        {
            n = *i;
            return true;
        }
    }
    return false;  // Not found
}