OO Arrays

spadgas1

Firstish post, so I expect you'll be cruel.

Anyway, I was looking at part of our codebase and I found this (C++)

typedef std::vector<int> DATA;

class Base {
public:
    Base() {};
    virtual ~Base() {};
    virtual void FillIn_Data() = 0;
};

class Culprit1_Data : public Base {
    public: Culprit1_Data() {}; 
    virtual ~Culprit1_Data() {};
    void FillIn_Data() {
        culprit1_data.push_back(VALUE1);
        //repeat for several different values
        culprit1_has_data = true;
    }

   static DATA culprit1_data;
   static bool culprit1_has_data;
};

//omitted class Culprit2 with members culprit2_data and culprit2_has_data and a different list of values to push back.
//omitted swathes of other code

class Culprit1 : public Other_Class
{
....
    void set_Culprit1(int thing)
    {
        if (!Culprit1_Data::culprit1_has_data) {
            Base *pCulprit1_Data = 0;
            pCulprit1_Data = new Culprit1_Data;
            pCulprit1_Data->FillIn_Data();
            delete pCulprit1_Data;
        }
        bool found = false;
        if (Culprit1_Data::culprit1_has_data){
            DATA::iterator p;
            for (p = Culprit1_Data::culprit1_data.begin(); p != Culprit1_Data::culprit1_data.end(); p++) {
                if (*p -- thing) { found = true; break; }
       }
      if (found) data_ = thing else throw invalid_param_exception();
     }
       //some other stuff of no interest
  private:
    int data_;
}

and yes. There's a Class2 which is almost indistinguishable from Class1, apart from the using Culprit2 instead of Culprit1.

Apparently defining an array of ints was too inflexible, or not OO enough or something.

PS - feel free to re-indent it - it was indented when I got it, but as I said, this is my first post.

Done. Removed the code tags, replaced them with pre's. Thank you for not using the code BB-code, thank you for having indentation already. --Ling

toth

Do I detect global variables? Dear God, tell me those classes aren't modifying global variables.

spadgas1

@toth said:

Do I detect global variables? Dear God, tell me those classes aren't modifying global variables.

They're public static class members. So definitely not global variables. Perish the thought.

PJH

@spadgas said:

@toth said:
Do I detect global variables? Dear God, tell me those classes aren't modifying global variables.
They're public static class members. So definitely not global variables. Perish the thought.

The next best thing to globals then?

@spadgas said:

if (!Culprit1_Data::culprit1_has_data) {
    Base *pCulprit1_Data = 0;
    pCulprit1_Data = new Culprit1_Data;
    pCulprit1_Data->FillIn_Data();
    delete pCulprit1_Data;
}

This bit is enough for me.

Where exactly is culprit1_has_data initialized?

arty

I once ran into some code like this in an MMO client. I have no idea how somebody could do const correctness, proper use of private and public and use STL and then write this...

class Packet {

public:

Packet(const char *data, int len) {

mydata.resize(len);

memcpy(&mydata[0], data, len);

}

SomeData *parse();

private:

static std::vector<char> mydata;

};

...

// std::list<Packet> recv_packet_queue; // STL is not thread safe!

void HandlePacketData(const char *data, int len) {

Packet p(data,len);

// Have to handle here due to threading issues

SomeData *s = p.parse();

s->perform();

...

}

Mole

defining a type called 'DATA' ? Perfect.

and I like the class called 'Base'.

and the many other WTFs...

How did this blob of sludge pass code review?

dhromed

@Mole said:

How did this blob of sludge pass code review?

Pass the what?

Mole

You mean there are companies out there which base there policy on production rather than trying to find out how many meetings they can cram into an 12 hour working day? :-o

If so, please let me know who they are so I can polish and send my résumé.

EDIT: Accidentally typed "8 hour day". Of course I mean "12 hour day". No one works 8 hour days anymore.

spadgas1

@Zecc said:

Where exactly is culprit1_has_data initialized?

There's a static initialiser I left out. It's C++ so there has to be one. And the class wasn't really called Base. It was slightly more meaningful.

Code review = 'does it work? OK then'. Some code reviews don't involve even that...

Mole

Our code reviews are just to ensure we follow the coding policy, and second, that we can state that we comply with all the requirements. It's not perfect (I've pointed to a printf statement before to satisfy a "must log" blah blah requirement and they were ok with it) but it keeps them happy and gets another tick on the documentation.

@spadgas said:

@Zecc said:
Where exactly is culprit1_has_data initialized?
There's a static initialiser I left out. It's C++ so there has to be one.

A repeated declaration, sure — otherwise you'd get an undefined reference — but not necessarily an initial value.

But I'll assume it's being set to false.