job interview question in C++

[yuvalif]

Hi All,
The company I work for is requiting a C++ developer to my team. I
prepared the following "test" and I would like to hear some comments
about it: is it too hard/easy, if its too specific, is it really
testing ones knowledge in C++, etc.
It is as following:

(1) Explain the following code, what it does? What will be the output
of "main"?
(2) Write a copy constructor for class "Values".
(3) What is the problem with task (2), change the code to make it
possible.

#include <vector>
#include <iostream>

using namespace std;

class BaseValue
{
public:
BaseValue() : m_val(0) {}
BaseValue(const BaseValue &newValue) : m_val(newValue.m_val) {}
virtual ~BaseValue() {}

void SetValue(int val) { m_val = val; }
virtual int GetValue() const = 0;
protected:
int m_val;
};

class Value2 : public BaseValue
{
public:
Value2() : BaseValue() {}
Value2(const Value2 &newValue) : BaseValue(newValue) {}
virtual int GetValue() const { return m_val*2; }
virtual ~Value2() {}
};

class Value4 : public BaseValue
{
public:
Value4() : BaseValue() {}
Value4(const Value4 &newValue) : BaseValue(newValue) {}
virtual int GetValue() const { return m_val*4; }
virtual ~Value4() {}
};

class Values
{
public:
Values(const char *sValList)
{
char *tmpList = (char*)malloc(strlen(sValList));
strcpy(tmpList, sValList);
char *token = strtok(tmpList, ",");
while(token)
{
int type = atoi(token);
if (type == 2)
m_values.push_back(new Value2());
else if (type == 4)
m_values.push_back(new Value4());
token = strtok(NULL, ",");
}
}

Values(const Values &newValues)
{
for(vector<BaseValue*>::const_iterator it =
newValues.m_values.begin();
it != m_values.end(); ++it)
{
m_values.push_back(new BaseValue(*it));
}
}

~Values()
{
for (vector<BaseValue*>::const_iterator it = m_values.begin();
it != m_values.end(); ++it)
{
delete *it;
}
}

void SetValue(int val, unsigned int pos)
{
// some validations are required here
m_values[pos]->SetValue(val);
}

void Dump() const
{
for (vector<BaseValue*>::const_iterator it = m_values.begin();
it != m_values.end(); ++it)
{
cout << (*it)->GetValue() << endl;
}
}

private:
vector<BaseValue*> m_values;
};
int main()
{
Values values("2,4,4,2,4");

values.SetValue(11,0);
values.SetValue(11,2);

values.Dump();

return 0;
}

[mlimber]

yu*****@gmail.com wrote:

Hi All,
The company I work for is requiting a C++ developer to my team. I
prepared the following "test" and I would like to hear some comments
about it: is it too hard/easy, if its too specific, is it really
testing ones knowledge in C++, etc.

[snip]

First of all, I'd switch from C-style strings to std::string. In place
of strtok, you could use the Boost tokenizer library:

http://boost.org/libs/tokenizer/index.html

If that's not fair game in an interview setting, I'd change the program
to do something else.

Cheers! -M

[deane_gavin]

mlimber wrote:

yu*****@gmail.com wrote:

Hi All,
The company I work for is requiting a C++ developer to my team. I
prepared the following "test" and I would like to hear some comments
about it: is it too hard/easy, if its too specific, is it really
testing ones knowledge in C++, etc. [snip]

First of all, I'd switch from C-style strings to std::string.

Indeed. I'd hope that, after skimming the code but before any serious
analysis, that would be one of the interviewees first reactions.
In place of strtok, you could use the Boost tokenizer library:

http://boost.org/libs/tokenizer/index.html

The smart pointers library will probably help too.

If the OP does insist on using C-style strings, memory allocation etc,
the appropriate headers will need to be included.

Oh, and there was an atoi in there to. Get rid of that (unless it's one
of the things the interviewee is supposed to spot).

Gavin Deane

[Christopher Benson-Manica]

yu*****@gmail.com wrote:

(3) What is the problem with task (2), change the code to make it
possible.
I'm curious - what is it?
Values(const char *sValList)
{
char *tmpList = (char*)malloc(strlen(sValList));
strcpy(tmpList, sValList);
char *token = strtok(tmpList, ",");
while(token)
{
int type = atoi(token);
if (type == 2)
m_values.push_back(new Value2());
else if (type == 4)
m_values.push_back(new Value4());
token = strtok(NULL, ",");
}
}

You've got a memory leak here, which is one of many excellent
arguments for not using malloc() in this language. Not to mention
gratuitous indentation...

--
Christopher Benson-Manica | I *should* know what I'm talking about - if I
ataru(at)cyberspace.org | don't, I need to know. Flames welcome.

[BigBrian]

Christopher Benson-Manica wrote:

yu*****@gmail.com wrote:

(3) What is the problem with task (2), change the code to make it
possible.

I'm curious - what is it?

Here's the copy constructor that the original post had.

Values(const Values &newValues)
{
for(vector<BaseValue*>::const_iterator it =
newValues.m_values.begin();
it != m_values.end(); ++it)
{
m_values.push_back(new BaseValue(*it));
}
}

I don't know if this is the issue or not, but I'm guessing the
following: Values basically encapsolates a vector<BaseValue*>. Since
BaseValue is subclassed and has virtual functions, my assumption is
that the intent of Values is to store a vector of pointers to BaseValue
which can be used polymorphically ( eventhough it doesn't provide this
in it's interface ). Maybe I'm wrong with this assumption, but if this
is not the intent than why subclass from BaseValues? Thus, if the
intent is to use what's stored in m_values polymorphically, then the
copy constructor above has a problem in that m_values in the copy
contains only pointers to BaseValue even if the object it's copying
contains pointers to instances of derivied classes, but just stored as
pointers to base in m_values. The BaseValue class needs a virtual
clone method, which can be overriden by the subclasses to return the
correct type so m_values can be copied correctly.

-Brian

[Axter]

BigBrian wrote:

Christopher Benson-Manica wrote:

yu*****@gmail.com wrote:

(3) What is the problem with task (2), change the code to make it
possible.

I'm curious - what is it?

Here's the copy constructor that the original post had.

Values(const Values &newValues)
{
for(vector<BaseValue*>::const_iterator it =
newValues.m_values.begin();
it != m_values.end(); ++it)
{
m_values.push_back(new BaseValue(*it));
}
}

I don't know if this is the issue or not, but I'm guessing the
following: Values basically encapsolates a vector<BaseValue*>. Since
BaseValue is subclassed and has virtual functions, my assumption is
that the intent of Values is to store a vector of pointers to BaseValue
which can be used polymorphically ( eventhough it doesn't provide this
in it's interface ). Maybe I'm wrong with this assumption, but if this
is not the intent than why subclass from BaseValues? Thus, if the
intent is to use what's stored in m_values polymorphically, then the
copy constructor above has a problem in that m_values in the copy
contains only pointers to BaseValue even if the object it's copying
contains pointers to instances of derivied classes, but just stored as
pointers to base in m_values. The BaseValue class needs a virtual
clone method, which can be overriden by the subclasses to return the
correct type so m_values can be copied correctly.

Forcing the class to have a clone function is one method, but you don't
have to code it that way to get the desired results.
You could use a clone smart pointer similar to the following:
http://code.axter.com/copy_ptr.h
or a COW pointer like the following:
http://code.axter.com/cow_ptr.h

Both of these smart pointers don't require the target class to have a
clone function, as long as the correct type is pass to the smart
pointer constructor.
for(vector<cow_ptr<BaseValue> >::const_iterator it =
newValues.m_values.begin();
it != newValues.m_values.end(); ++it)
{
m_values.push_back(*it);
}

or better yet:
Values(const Values &newValues):m_values(newValues.m_values)
{ //Where m_value type is vector<cow_ptr<BaseValue> >
}
I recommend this method over the clone function method, because it can
be used more generically, in that it doesn't force a function on the
base class, and/or derived class.
FYI:
Both methods can fail when using a derived-derived class, but the clone
method requires more coding for each derived class, and IMHO, is easier
to get it wrong in that you can forget to add the requirer clone method
in a derived-derived class.

[BigBrian]

Axter wrote:

BigBrian wrote:

Christopher Benson-Manica wrote:

yu*****@gmail.com wrote:

> (3) What is the problem with task (2), change the code to make it
> possible.

I'm curious - what is it?

Here's the copy constructor that the original post had.

Values(const Values &newValues)
{
for(vector<BaseValue*>::const_iterator it =
newValues.m_values.begin();
it != m_values.end(); ++it)
{
m_values.push_back(new BaseValue(*it));
}
}

I don't know if this is the issue or not, but I'm guessing the
following: Values basically encapsolates a vector<BaseValue*>. Since
BaseValue is subclassed and has virtual functions, my assumption is
that the intent of Values is to store a vector of pointers to BaseValue
which can be used polymorphically ( eventhough it doesn't provide this
in it's interface ). Maybe I'm wrong with this assumption, but if this
is not the intent than why subclass from BaseValues? Thus, if the
intent is to use what's stored in m_values polymorphically, then the
copy constructor above has a problem in that m_values in the copy
contains only pointers to BaseValue even if the object it's copying
contains pointers to instances of derivied classes, but just stored as
pointers to base in m_values. The BaseValue class needs a virtual
clone method, which can be overriden by the subclasses to return the
correct type so m_values can be copied correctly.

Forcing the class to have a clone function is one method, but you don't
have to code it that way to get the desired results.
You could use a clone smart pointer similar to the following:
http://code.axter.com/copy_ptr.h
or a COW pointer like the following:
http://code.axter.com/cow_ptr.h

Both of these smart pointers don't require the target class to have a
clone function, as long as the correct type is pass to the smart
pointer constructor.

Interesting,
Both methods can fail when using a derived-derived class, but the clone
method requires more coding for each derived class, and IMHO, is easier
to get it wrong in that you can forget to add the requirer clone method
in a derived-derived class.

That's a good point.

[yuvalif]

Thanks all for the responses.
I'll fix the memory leak and "atoi" call, but I'll think I'll stay with
"strtok", I don't think I can expect someone to know anything else (I
didn't).
Regarding the solutions, I'm glad there is more than one, and that they
are at different levels, I thought a person that will understand the
problem is "reasonable", someone that will give an "ugly" type/enum
oriented solution is "quite good", and someone that give the virtual
"clone" solution is "good" - that is what I did :-)
If there is a better "smart pointer" solution, well...

Yuval.

[bob]

whats wrong with the indentation?

[Mirek Fidler]

> (1) Explain the following code, what it does? What will be the output

of "main"?
The code leaks memory and occasionally crashes:
Values(const char *sValList)
{
char *tmpList = (char*)malloc(strlen(sValList));
strcpy(tmpList, sValList);
char *token = strtok(tmpList, ",");
while(token)
{
int type = atoi(token);
if (type == 2)
m_values.push_back(new Value2());
else if (type == 4)
m_values.push_back(new Value4());
token = strtok(NULL, ",");
}
}

- tmpList is never freed
- there is no place for terminating '\0' allocated in tmpList

Mirek

[Jay Nabonne]

On Thu, 22 Dec 2005 07:46:26 -0800, yuvalif wrote:

class Values
{
public:
Values(const char *sValList)
{
char *tmpList = (char*)malloc(strlen(sValList));
char *tmpList = (char*)malloc(strlen(sValList)+1);
strcpy(tmpList, sValList);

Or just use strdup (if you really want to use this kind of code).

- Jay

[roberts.noah]

yu*****@gmail.com wrote:

Thanks all for the responses.
I'll fix the memory leak and "atoi" call, but I'll think I'll stay with
"strtok", I don't think I can expect someone to know anything else (I
didn't).

On the other hand, a *C++* pogrammer may not be very experienced with
strtok.

[BobR]

yu*****@gmail.com wrote in message
<11**********************@o13g2000cwo.googlegroups .com>...

Thanks all for the responses.
I'll fix the memory leak and "atoi" call, but I'll think I'll stay with
"strtok", I don't think I can expect someone to know anything else (I
didn't).

#include <sstream> // C++
// .... [ "strtok"? I don't need no stinkin' "strtok"! (or malloc) ]
Values(const char *sValList){
std::istringstream tokens(sValList);
int Type(0);
while( tokens ){
tokens >> Type;
if(Type == 2 ){ m_values.push_back(new Value2());}
if(Type == 4 ){ m_values.push_back(new Value4());}
tokens.ignore(1, ','); Type=0;
} //while()
} //Values(const char*)
// ....
int yuvalif_main(std::ostream& cout= std::cout){
Values values("2,4,4,2,4");
values.SetValue(11,0);
values.SetValue(11,2);
values.Dump(cout); // Dump(std::ostream& cout=std::cout) const {}
return 0;
} //main() end
/* - output -
22
0
44
0
0
*/

So, what's the job pay? <G>
--
Bob R
POVrookie

[Christopher Benson-Manica]

bo*@blah.com <Gr**********@gmail.com> wrote:

whats wrong with the indentation?

It's rather excessive.

--
Christopher Benson-Manica | I *should* know what I'm talking about - if I
ataru(at)cyberspace.org | don't, I need to know. Flames welcome.