8
\$\begingroup\$

I want to parse a json file, where some elements are common each time while others are different based on a type. For example a message json:

{
    "title": string,
    "body": string,
    "type": int (0 for email, 1 for sms, 2 for terminal)
    "typeProperties": {
        // Different based on type value
    }
}

I ended up to the following code to represent the json object:

#include <string>
#include <vector>
#include <iostream>

class Message {
    public:
        int id;
        std::string title;
        std::string body;
        int type; // 0 - Email, 1 - SMS, 2 - Terminal
};

class Email : public Message {
    public:
        std::string emailRecipients;
        std::string emailCC;
};
class Sms : public Message {
    public:
        std::string  phoneNumber; 
};
class Terminal : public Message {
    public:
        int terminalId;
};

std::vector<Message*> list_msg;

int main(int argc, char** argv) {
    Email mail;
    mail.id = 1;
    mail.title = "title1";
    mail.body = "body1";
    mail.type = 0;
    mail.emailRecipients = "text1";
    mail.emailCC = "text2";

    list_msg.push_back(&mail);

    Terminal tms;
    tms.id = 2;
    tms.title = "title2";
    tms.body = "body2";
    tms.type = 1;
    tms.terminalId = 123;

    list_msg.push_back(&tms);

    for (auto m : list_msg) {
        std::cout << "id: " << m->id << ", type: " << m->type << std::endl;

        if (m->type == 0) {
            Email *tmp = static_cast<Email*>(m);
            std::cout << ">>>> recipient: "  << tmp->emailRecipients << ", cc: " << tmp->emailCC << std::endl;
        } else if (m->type == 1) {
            Terminal *tmp = static_cast<Terminal*>(m);
            std::cout << ">>>> terminalId: "  << tmp->terminalId << std::endl;
        }
    }
}

Questions:

  1. Is the above code correct?
  2. From performance perspective, can this be optimized?
  3. If I don't use inheritance and have one class Message with a boost::variant member - what are the pros and cons of this approach?
\$\endgroup\$
  • \$\begingroup\$ Is the order of elements fixed or can they be mixed in input file? Also I believe there is no parsing involved in the code? \$\endgroup\$ – Incomputable Jul 12 '17 at 12:42
  • \$\begingroup\$ The order of elements are fixed. Also, a json file can represent only one message type. My intention here is the best representation in C++ classes of a json file where some elements of the json can be different depending the type. \$\endgroup\$ – georgeliatsos Jul 13 '17 at 7:42
  • 1
    \$\begingroup\$ the last thing I would make is to have factory function and implement it somewhere else to loosen coupling. \$\endgroup\$ – Incomputable Jul 13 '17 at 18:08
3
\$\begingroup\$

Correctness

I'm not sure what exactly you mean by 'correct'. Personally, I regard this code as poorly written, so I would reject it during peer review at my work. It's using C++ as if it were C, and violating several OOA/D heuristics.

Class Message knows it has subclasses

Class Message shouldn't have that 'type' member - a base class should not know anything about its subclasses, not even that any subclasses exist. At the moment, class Message knows that it has <= 2^(sizeof(int)*8 - 1) subclasses (else that int would need to be a 'uint128_t' or something). And if you changed that 'int' to be an 'enum message_type', which is what it really is, then Message would know of the existence of each of its subclasses, which would be even worse: addition of a subclass would require modifying the base class.

It seems to me you can simply delete that 'type' member, and the lines that set its value, without any loss of functionality.

The take-away point is: C++ objects (instances of some class) already 'know' the class of which they are an instance. You don't need to store this information a second time using class data members.

Encapsulation (Google for this term)

Classes shouldn't have public data - where you're setting public members directly, you should either be passing arguments to constructors, or (less preferably) calling setters after instance construction.

Performance

Do you really care about performance of this code? Think very hard about why this code is performance-critical. Chances are, it's not.

If this code really is performance-critical, for example it's executed thousands of times per second on a small embedded processor in a real-time system on which lives depend, then consider instead using an optimised binary format instead of JSON, using superior algorithms elsewhere in your code, offloading work to other parts of the system that are less time-critical or have more resources, or simply reducing the scope of the project to something easier to accomplish.

Pros and cons of using inheritance (instead of a variant member)

Pros of inheritance: Uses C++ in the way its designers intended and in the way later readers will expect. Allows for polymorphism (Google for this term). Promotes future code re-use and eases modification, by making it easier to add additional subclasses later on, or modify the superclass behaviour/implementation.

Cons of inheritance: I can't think of any. You'd be mimicking inheritance by use of containment, which may be sadly necessary in some circumstances but seems worthless to you in this circumstance.

Strange JSON

The JSON is redundant, which means it's possible for it to be inconsistent. What does it mean for the JSON to contain a 'type' attribute with the value 0, meaning email, but then inside its 'typeProperties' sub-object, it contains all of the the attributes appropriate to an SMS and none of the attributes appropriate to an email?

If this isn't possible, then why does the JSON design permit it? Why not just treat it as an SMS if it has SMS members, or an email if it has email members, and dispense with the 'type' attribute?

If this is possible, then how are you intending to handle it?

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

Is the above code correct?

It compiles and works like expected, thus I would say it is correct.

On first view I had guessed that you have a memory leak because you do not delete the pointer in list_msg but than I saw that they are not created with new, so everything is fine.

From performance perspective, can this be optimized?

The program is not very long and has not that much work to do, thus it would be hard to optimize because everything you do will have only minimal impact.

But if you really want to go for optimization than look within the most frequent loops (here your only for-loop) and go for the obvious. All other code optimizations should be for clarity and only if profiling information is available for performance.

Nevertheless one thing I noticed: within your loop you use std::endl. This is an obvious performance issue (in frequent loops) because it not only creates a new line, it also flushes the stream which is not the fastest operation. Replace it with "\n" (or '\n') and you will get better performance without changing the clarity of the code.

If I don't use inheritance and have one class Message with a boost::variant member - what are the pros and cons of this approach?

In my opinion there are not so much pros for this (in this case). Once I used a variant struct in similar cases (a QVariant) for the reason that it creates less code (fewer classes) and thus would be better. I was wrong. It only creates code where you can bypass the compiler type-checking. We used it in a small program which than got bigger with time (surprise). In the end, when I changed the type of some container (set to vector), I could not trust the compiler errors to find all places where this type was used. In my opinion it also reduces code readability, which is never good.


Some more things

Global Variables

I do not see a reason to make list_msg a global variable. Just move it to the beginning of main.

Abstraction

Even if I see your reason to use inheritance for the message, I do not see a reason to place them all into one single vector. The usual reason for such a thing would be that you will use them somewhere in a similar way which is specialised by the derived classes.

Example: If you move the printing into a member function of Message virtual void printSpecialInfo() = 0; and use that function within the loop you would have "real" inheritance. Your way of type-switching is not really the c++-way of doing things. One reason is that the id duplicates the logic of which type you have and has a tendency to force you to the same if(id==XXX) all over the code.

Another reason: If you do not abstract-away the class information (use separate vectors), you could omit the pointers, which could improve readability (and performance, since pointer lookup may be more expensive than direct invocation of functions).

Factory Function

I would separate the creation of Messages into a function. Something like std::unique_ptr<Message> readMessageFromJson(Json x);.

\$\endgroup\$

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.