yet another beginner's design question

mzimmers

I'm writing an app that communicates to a faceless embedded device for the purpose of provisioning/configuration. I'm comfortable with C++ coding but relatively new to the practice of OOD.

I've created two classes: Worker and Widget. Within the Worker I have:

Device: holds information about the device (obtained from communication with the device)
Serial: handles the actual low level serial I/O with the remote device
Message: formats outgoing messages and helps interpret incoming messages.

Currently, a Serial slot reads an incoming message and the Worker parses it. Based on the kind of message, it determines a response. Much of the response is information from the Device object.

My question is: is it "correct" (from an OOD perspective) to pass an entire QHash of information from the Worker to the Message? As the Message and Device classes are peers, they don't have a way to communicate directly, at least not that I can see. I could write a bunch of get/set methods for the Device, but that's a lot of programming. I don't mind doing it, but I wanted to sanity check my design before I launch into all that.

Thanks for any suggestions.

ambershark

@mzimmers Well to me, Device/Serial/Message would all be classes that are outside of Worker. If Worker needs them, it would stand to reason some other part of the application would as well. By containing them (at least privately) inside Worker you are limiting their reuse.

Other than that I don't see any glaring issue with your design.

As for the QHash thing, I tend to use get/set methods. If you have data that both the device and message need then I would make another object, we'll call it Data, and I would then set that up in Device or whoever gets it first, then you can pass that entire Data object (by const reference or pointer for speed and memory usage) to your Message object to be handled.

It's not like the QHash won't work but it would be confusing for any other developer or even you in a few years as to what it represented and what you were doing with it. Part of good design/coding, at least imo, is simplicity. Another developer should be able to pick up your code and maintain it very easily. If they are confused you probably had bad design.

I work with a lot of people that seem to think their intelligence or success as a coder is to do the newest/coolest/most complicated thing they can. So I see all kinds of really complicated code for a very simple idea. Needless to say their code has way more bugs than my "simple" code, and it all does the same thing. Simple may involve a bit more typing though, but you make up for it on the back end when you have to maintain it. ;)

mzimmers

@ambershark thanks for the reply.

The object you call "Data" sounds very much like my "Device" object...I probably could have picked a better name.

My worker object is kind of the master object for all things not UI related. It's primary purpose, really, is to keep main() to a minimal amount of code. If I make the other objects peers to the worker, how do they exchange information? For example, who would invoke the message formatter if not the worker?

I tend to agree with your statement regarding using gets/sets vs. passing in an entire data structure; I just wanted to hear from someone with more experience.

And, I agree completely about avoiding the tendency to go for the "Next Best Thing." I'm trying to use good design practices, and take full advantage of OOP principles, but I'm not trying to be fancy or clever. If I just wanted to get this going, I'd be done by now, but I'm trying to "do it right."

ambershark

@mzimmers said in yet another beginner's design question:

@ambershark thanks for the reply.

The object you call "Data" sounds very much like my "Device" object...I probably could have picked a better name.

No I'm sure your name is fine, I just don't really have enough details to understand everything right now.

My worker object is kind of the master object for all things not UI related. It's primary purpose, really, is to keep main() to a minimal amount of code. If I make the other objects peers to the worker, how do they exchange information? For example, who would invoke the message formatter if not the worker?

Even if Worker is a global level singleton type class you can still have things like Device and Message be peers. Ideally classes should be encapsulated and only really rely/know about themselves. Sometimes it's inevitable that you couple some classes to others but you really want to avoid doing that.

So Worker can still instantiate an instance of Device and pass it to Message. Then Message would know about Device without needing to do something like void Message::doSomething(Worker::Device *myDevice); You wouldn't need the public class definition inside Worker if they were peers.

You would still have device be something with getters and setters like:

class Device
{
public:
   Device() : thing_(0) { }

   //...

   void setSomeThing(int x) { thing_ = x; }
   int someThing() const { return thing_; }

private:
   int thing_;
};

I tend to agree with your statement regarding using gets/sets vs. passing in an entire data structure; I just wanted to hear from someone with more experience.

I mean it doesn't hurt to use a data structure, but at that point you probably need a class to manage all the data, and again you would just pass that class. You would still use get/sets inside that class though.

And, I agree completely about avoiding the tendency to go for the "Next Best Thing." I'm trying to use good design practices, and take full advantage of OOP principles, but I'm not trying to be fancy or clever. If I just wanted to get this going, I'd be done by now, but I'm trying to "do it right."

Oh that last part was just an anecdote about how quickly bad design can sneak up on coders because they are trying to over engineer something. It had nothing to do with you or your question really. :)

kshegunov

Think carefully about what class does what exactly and name the class accordingly. A class should do one thing and one thing alone. From your description I'd say your Serial class should rather be called SerialDevice and be a subset of a more general Device class - yes I'm thinking about subclassing an interface here.

Then the question follows - what does your Device class do? Should it just buffer the data, then I'd call it MessageBuffer or something alike, to reflect that. If it does parsing, then it's a parser, but generally keep the buffer separated from the parser, they'd have to be separate classes.

And finally, Message, what does it do? Is it just a container for raw data? If so then I'd go with a copyable type (e.g. one that implements implicit sharing, see QSharedData and QSharedDataPointer classes) that would be marshaled around. If the formatting/serialization is done, then it'd be a matter of its dependencies:

Does the serialization depend on the Device? If so, then a utility class that serializes messages for that device is in order.
If it doesn't, then the encoding/decoding can be placed in the message itself by introducing a QDataStream and/or QTextStream operators which can be used by the device to initiate the serialization/deserialization process.

There's some leeway here too, because the messages may be complicated (i.e. they may be implemented by different classes that have specific formats). In this case a virtual streaming operators would be a solution of choice. You also may be interested how I'd implemented it here for my QtMpi module.

mzimmers

@kshegunov agreed about the Serial class, but I think I'll leave it for now. I can always build a superset class for it later.

The Device class probably should be renamed; it just contains information about the connected device, and makes this information available via gets/sets.

The Message class creates responses (on behalf of the Worker). This falls into two primary activities: creating the content of the response, and then formatting it for output.

It sounds like if anything should be shared, it should be Device. I'm not familiar with QSharedData...will using this make passing an object as an argument more efficient?

Ambershark: your suggestion of passing objects as arguments was really what helped solidify this design -- thanks for that.

kshegunov

@mzimmers said in yet another beginner's design question:

This falls into two primary activities

Then you need two classes ;)
That was my point, one class does one thing.

It sounds like if anything should be shared, it should be Device. I'm not familiar with QSharedData...will using this make passing an object as an argument more efficient?

The two classes are used to make data classes with a lightweight copy-on-demand. Qt's container and data classes are all done like this, which include but not limited to QString, QVector, QSet, QHash, QByteArray and so on. And yes if you have a heavy data-containing class then you should seriously consider using this to not cause excessive copies (e.g. when it's used with queued signal-slot connections).

mzimmers

@kshegunov got it...thanks for the clarification.