1
\$\begingroup\$

About a year ago, I was given a C++ assignment as a preliminary job-interview test.

The assignment included 4 different tasks, from which I had to choose one.

I actually chose and completed 3 out of 4 tasks, but I did not pass the test.

I was not given any specific rejections, so I would to like get an unbiased opinion.


enter image description here

enter image description here


My answer:

File main.cpp:

#include <map>
#include <vector>
#include <sstream>
#include <fstream>
#include <iostream>

#include "ModuleNoop.h"
#include "ModuleEcho.h"
#include "ModuleDelay.h"
#include "ModuleReverse.h"

static void Run(istream& input);

int main(int argc,char* argv[])
{
    if (argc > 1)
        Run(ifstream(argv[1])); // read line by line from an input file
    else
        Run(cin); // read line by line from stdin (i.e., from the keyboard)
    return 0;
}

static void Run(istream& input)
{
    map<string,int> indices;
    vector<Module*> modules;

    while (!input.eof())
    {
        string keyword;
        input>>keyword;
        if (keyword.compare("module") == 0)
        {
            string name;
            string operation;
            input>>name>>operation;
            Module* module = nullptr;
            if (operation.compare("noop") == 0)
                module = new ModuleNoop;
            else if (operation.compare("echo") == 0)
                module = new ModuleEcho;
            else if (operation.compare("delay") == 0)
                module = new ModuleDelay;
            else if (operation.compare("reverse") == 0)
                module = new ModuleReverse;
            if (module)
            {
                indices[name] = modules.size();
                modules.push_back(module);
            }
        }
        else if (keyword.compare("connect") == 0)
        {
            string name1;
            string name2;
            input>>name1>>name2;
            modules[indices[name1]]->Connect(modules[indices[name2]]);
        }
        else if (keyword.compare("process") == 0)
        {
            string line;
            getline(input,line);
            cout<<modules[0]->Process(line);
            cout<<modules[0]->Process();
        }
    }

    for (auto ptr=modules.begin(); ptr!=modules.end(); ptr++)
        delete *ptr;
}

File Module.h:

#ifndef MODULE_H
#define MODULE_H

#include <string>
using namespace std;

class Module
{
public:
    Module();
    virtual ~Module();
public:
    void Connect(Module* next);
    string Process(const string& line="");
protected:
    virtual string ProcessWord(const string& word) = 0;
    Module* next;
};

#endif

File Module.cpp:

#include "Module.h"
#include <sstream>

Module::Module():next(nullptr)
{
}

Module::~Module()
{
}

void Module::Connect(Module* next)
{
    this->next = next;
}

string Module::Process(const string& line)
{
    istringstream iss(line);
    ostringstream oss;
    while (!iss.eof())
    {
        string word;
        iss>>word;
        oss<<' '<<ProcessWord(word);
    }
    if (next)
        return next->Process(oss.str());
    return oss.str();
}

File ModuleNoop.h:

#ifndef MODULE_NOOP_H
#define MODULE_NOOP_H

#include "Module.h"

class ModuleNoop : public Module
{
public:
    ModuleNoop();
    virtual ~ModuleNoop();
private:
    string ProcessWord(const string& word);
};

#endif

File ModuleNoop.cpp:

#include "ModuleNoop.h"

ModuleNoop::ModuleNoop()
{
}

ModuleNoop::~ModuleNoop()
{
}

string ModuleNoop::ProcessWord(const string& word)
{
    return word;
}

File ModuleEcho.h:

#ifndef MODULE_ECHO_H
#define MODULE_ECHO_H

#include "Module.h"

class ModuleEcho : public Module
{
public:
    ModuleEcho();
    virtual ~ModuleEcho();
private:
    string ProcessWord(const string& word);
};

#endif

File ModuleEcho.cpp:

#include "ModuleEcho.h"

ModuleEcho::ModuleEcho()
{
}

ModuleEcho::~ModuleEcho()
{
}

string ModuleEcho::ProcessWord(const string& word)
{
    return word+' '+word;
}

File ModuleDelay.h:

#ifndef MODULE_DELAY_H
#define MODULE_DELAY_H

#include "Module.h"

class ModuleDelay : public Module
{
public:
    ModuleDelay();
    virtual ~ModuleDelay();
private:
    string ProcessWord(const string& word);
    string curr;
};

#endif

File ModuleDelay.cpp:

#include "ModuleDelay.h"

ModuleDelay::ModuleDelay():curr("hello")
{
}

ModuleDelay::~ModuleDelay()
{
}

string ModuleDelay::ProcessWord(const string& word)
{
    string prev = curr;
    curr = word;
    return prev;
}

File ModuleReverse.h:

#ifndef MODULE_REVERSE_H
#define MODULE_REVERSE_H

#include "Module.h"

class ModuleReverse : public Module
{
public:
    ModuleReverse();
    virtual ~ModuleReverse();
private:
    string ProcessWord(const string& word);
};

#endif

File ModuleReverse.cpp:

#include "ModuleReverse.h"

ModuleReverse::ModuleReverse()
{
}

ModuleReverse::~ModuleReverse()
{
}

string ModuleReverse::ProcessWord(const string& word)
{
    string drow;
    for (int i=word.length()-1; i>=0; i--)
        drow += word[i];
    return drow;
}

Tested input:

module alpha reverse
module beta delay
connect alpha beta
process hello world
\$\endgroup\$
  • \$\begingroup\$ Well I would fail you for not following the instructions Complete 1 of the 4 tasks. \$\endgroup\$ – Martin York Mar 17 '15 at 21:50
  • \$\begingroup\$ Also I would double check your e-mail about weather the question is confidential. A lot of companies do not like their questions being spread across the internet (as it makes them useless) and is quite expensive to develop the test. \$\endgroup\$ – Martin York Mar 17 '15 at 21:52
  • \$\begingroup\$ @LokiAstari: Thanks for the advise. It did occur to me. The email does not say anything about confidentiality as far as I remember, but nevertheless, I wouldn't want to reveal this just for the sake of my own personal integrity (i.e., they gave me the opportunity, so I should respect that even if I did not pass the test for whichever reason). I therefore refrained from specifying the company's name anywhere within my questions. But I will reconsider moving it altogether if I get more comments suggesting that it is inappropriate for that matter. Thanks again. \$\endgroup\$ – barak manos Mar 17 '15 at 21:58
1
\$\begingroup\$

It appears that you overlooked the most important part of the assignment (it calls for network of transformations):

If there are multiple connections for a module, they should be summed before feeding to the module.

That clearly assumes that a module may have more than one incoming connection. Now the input is initially fed to just one module. In order to achieve miltiple incoming connections, modules must support multiple outgoing connections.

The code provides neither forking nor summing.

\$\endgroup\$
1
\$\begingroup\$

Reasons to reject you:

Classic anti pattern to read a file:

while (!input.eof())
{

This is wrong in every language. You attempt to read and see if it worked.
EOF is not set till you try and read past the EOF. The last succesful read will read up to (but not past the eof). So the next read will fail, but eof() is still false.

std::string keyword;
while (input>>keyword)    // But even that is limitting.
{                         // I would have had a "command object" that I read.

Using RAW pointers

Module* module = nullptr;

Obviously not an experienced C++ programmer. You are using a C style. Fail. I'll move onto the next candidate here.

Pointers should always be managed. Use a smart pointer (make sure you pick the appropriate one).

Also you are pushing the pointers into a non managed container.

vector<Module*> modules;

Again who owns the data. If the container ownes the data then you need to add manual code to destroy the objects.

Not using standard patterns

This looks like a classic case for the command pattern.

        if (operation.compare("noop") == 0)
            module = new ModuleNoop;
        else if (operation.compare("echo") == 0)
            module = new ModuleEcho;
        else if (operation.compare("delay") == 0)
            module = new ModuleDelay;
        else if (operation.compare("reverse") == 0)
            module = new ModuleReverse;

Or oven just a command object than can be serialized.

Not using standard C++ idioms.

class Module
{
public:
    Module();
    virtual ~Module();
public:
    void Connect(Module* next);
    string Process(const string& line="");
protected:
    virtual string ProcessWord(const string& word) = 0;
    Module* next;
};

There is a RAW pointer in this class. You don't obey the rule of three. Immediate fail. In the trash you are not a C++ programmer.

Summary

OK. Got bored. Sorry slightly irreverent towards the end. But there is plenty in here that shows you are not an experienced C++ programmer (you are basically using C with classes).

\$\endgroup\$
  • \$\begingroup\$ Right. That's a very negative attitude expressed in a single answer (even if it is 100% correct, which I am not entirely sure of). I would normally reply with gratitude, but not in this case! \$\endgroup\$ – barak manos Mar 17 '15 at 22:12
  • \$\begingroup\$ @barakmanos: Not hear to stroke your ego. You wanted to know why you were rejected. These are the reasons I would have rejected you for. And they are all correct. Given this I would just assume your experience with C++ is limited (are you a C programmer trying to migrate to C++). You have the potential to learn but this would not hack it in any serious company. \$\endgroup\$ – Martin York Mar 17 '15 at 22:30
  • \$\begingroup\$ Didn't ask for a stroke. Refrain from using words and expressions such as "I'll move onto the next candidate here", "In the trash you are not a C++ programmer", etc. Stick to constructive technical comments next time. That's what I'd expect from any decent answer here - no more, no less. \$\endgroup\$ – barak manos Mar 17 '15 at 22:37
  • \$\begingroup\$ @barakmanos: If you want to do a review of your own code fine. Just describing my actions. If you don't like my answer don't read it. \$\endgroup\$ – Martin York Mar 17 '15 at 22:41
  • \$\begingroup\$ First of all, I can't "not like" it without reading it, so your suggestion is an oxymoron. Describing your actions is fine. Your technical comments are also useful and well appreciated. Your personal negative thoughts are irrelevant, and quite frankly - inappropriate. \$\endgroup\$ – barak manos Mar 17 '15 at 22:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.