Design: Signals => Pass by Reference

[tescrin]

[Disclaimer: This is not syntactically correct. I just built enough to make the question clear]

Suppose I have a pair of classes:

class a{
//..
  signals:
    void ask_B_for_update(double);
 
  public slots:
    void update_A(double);
}
 
class b{
//..
  signals:
    void send_update_to_A(double);
 
  public slots:
    void update_B(double);
}
 
connect(ask_B_for_update,update_B)
connect(send_update_to_A,update_A)

class a{
//..
  signals:
    void ask_B_for_update(double);

  public slots:
    void update_A(double);
}

class b{
//..
  signals:
    void send_update_to_A(double);

  public slots:
    void update_B(double);
}

connect(ask_B_for_update,update_B)
connect(send_update_to_A,update_A)

To copy to clipboard, switch view to plain text mode 

This means that to update a value (in my case a keypad) from some kind of database I'll say "Please give me a maximum value"
"I'll go get it"
"Here you go"
"Ok now I can save it"

Were I to instead do:

class a{
//..
  signals:
    void ask_B_for_update(&double);
 
}
 
class b{
//..
  public slots:
    void update_A(&double);
}
 
connect(ask_B_for_update,update_A)

class a{
//..
  signals:
    void ask_B_for_update(&double);

}

class b{
//..
  public slots:
    void update_A(&double);
}

connect(ask_B_for_update,update_A)

To copy to clipboard, switch view to plain text mode 

This reduces code bloat and increases readability in my mind. Further, doing it as the first style one needs to save the result to a member variable instead of being able to utilize a local variable. This is wasteful and bad design in my mind. I'd note that COM, however dreadful it is to work with, tends to do the latter; and the design doesn't feel bad to use. While not important, this is also faster, less complex, etc..


Question:
It would seem from my search though that the only information I've found says that sending references by signals is bad. When doing so in the above we see that we (potentially..) halve the number of functions, we increase speed, we increase readability, we increase reliability (I think), and we reduce complexity.

What argument can stand to eliminate all of these benefits?


I'm looking for a mild debate (if you disagree with this) as I've designed initially in the first style. Now I see all of these benefits but don't wish to make a large design mistake and waste half a day (essentially) redoing work to get a worse product.


[NOTE: I should mention that I mean this primarily for simple data types (ints, doubles, etc..) ]

Thank you for your input

[wysota]

What argument can stand to eliminate all of these benefits?

If we connect two (or more) slots to the signal and both of them change the value, we lose one (or more) of the changes and only the first callee receives the original (unchanged) value.

To counter your upcoming argument that you are going to connect the signal to only one slot -- if you know who is going to be the receiver of your signal, you don't need a signal at all. Instead call the method in the receiver object directly. This will be even faster and less complex than using signals.

[amleto]

The other thing with pass by reference in signal/slots is that it is actually pass by value on queued connections! Sneaky, huh?

[wysota]

The other thing with pass by reference in signal/slots is that it is actually pass by value on queued connections! Sneaky, huh?

Actually it will probably bail out (because of unregistered meta-type) instead of working at all

[tescrin]

To counter your upcoming argument that you are going to connect the signal to only one slot -- if you know who is going to be the receiver of your signal, you don't need a signal at all. Instead call the method in the receiver object directly. This will be even faster and less complex than using signals.

I hear what you're saying, but the idea of directly calling breaks a lot of abstraction (and the possibility of extension) especially if programming in an EDP style. Also, if someone wanted to keep my GUI and but have a new backend, they'd have to edit in the GUI (which points to what would be an obvious flaw in the design.)

That said, you're right about if I hooked it up to multiple slots and I'm surprised I hadn't considered that. I'm just *really* anxious about storing member variables for these various pieces of data. It feels wrong to be storing state in my GUI portion of the code where I only need the state for initialization and at that, only need the state temporarily (grab the state, initialize my mins, maxs, default values, and what have you; then the value is deprecated at that point.)

I guess I'll note that in the final form it probably *will* have several slots connecting from one of the directions to the other; so I'll keep slogging on the current course.




Do you think it's a safe enough design (given that asynchronicity would probably break the design in it's current form anyway) then to have generic member vars (such as "current_min", "current_max", "current_default") and store the values there; to get away from the dirtiness of a bunch specific member vars (and excess member functions?) If not threaded/synchronous they should be updated correctly via the signals/slots and once those are completed we stuff those into the new dialog object.


I.E.

keypad_creator_Q()
{
  emit get_my_Q_values(); //updates m_min, m_max, and m_default via some generic slot
  new keypad(Q,m_min,m_max,m_default);
  keypad.exec();
  delete keypad;
  //..
}

keypad_creator_Q()
{
  emit get_my_Q_values(); //updates m_min, m_max, and m_default via some generic slot
  new keypad(Q,m_min,m_max,m_default);
  keypad.exec();
  delete keypad;
  //..
}

To copy to clipboard, switch view to plain text mode 

[wysota]

I hear what you're saying, but the idea of directly calling breaks a lot of abstraction (and the possibility of extension) especially if programming in an EDP style. Also, if someone wanted to keep my GUI and but have a new backend, they'd have to edit in the GUI (which points to what would be an obvious flaw in the design.)

It doesn't break anything if you provide an abstract interface the observer will implement and the notifier will use.

That said, you're right about if I hooked it up to multiple slots and I'm surprised I hadn't considered that. I'm just *really* anxious about storing member variables for these various pieces of data. It feels wrong to be storing state in my GUI portion of the code where I only need the state for initialization and at that, only need the state temporarily (grab the state, initialize my mins, maxs, default values, and what have you; then the value is deprecated at that point.)

I don't see how you need to be storing anything anywhere.

Do you think it's a safe enough design (given that asynchronicity would probably break the design in it's current form anyway) then to have generic member vars (such as "current_min", "current_max", "current_default") and store the values there; to get away from the dirtiness of a bunch specific member vars (and excess member functions?) If not threaded/synchronous they should be updated correctly via the signals/slots and once those are completed we stuff those into the new dialog object.


I.E.

keypad_creator_Q()
{
  emit get_my_Q_values(); //updates m_min, m_max, and m_default via some generic slot
  new keypad(Q,m_min,m_max,m_default);
  keypad.exec();
  delete keypad;
  //..
}

keypad_creator_Q()
{
  emit get_my_Q_values(); //updates m_min, m_max, and m_default via some generic slot
  new keypad(Q,m_min,m_max,m_default);
  keypad.exec();
  delete keypad;
  //..
}

To copy to clipboard, switch view to plain text mode 

Either I don't know how this is supposed to work or it's simply too complicated. I prefer to use a design similar to QNetworkAccessManager, QNetworkRequest and QNetworkReply. QNAM is the "doer", QNReq is the "task" and QNRep is the "result".

[tescrin]

In order to update via the first style one follows the logic I put forth earlier; meaning that the object that needs information receives it via a slot. This slot is not in the "local" scope and thus can't utilize your locals, so it would have to save to members (or some other class-wide variable.) This is what I mean.

While not probing directly there's no direct call that can be made, without utilizing references, to update a variable you only need once. I realize one can use interfaces to accomplish similar means but EDP allows for a simplified implementation object-wise in the rest of my solution*. This is why I mentioned the possibility of having member vars that represent standard data (such as mins/maxs) and it's the implementation I'm going with first as it minimizes the storing of state.

[*This is because the instrument I'm attaching to has 1000's of functions. To abstract this into logical chunks I'm breaking or combining their interfaces by building objects in an "Observer" or "View Model" style. This means that EDP is a natural fit as I'm unsure who I'll be talking to until I hook up (as much of my project is currently exploratory. The instrument in question was a somewhat recent acquisition.) ]


I don't think it's too complex :S. If the GUI needs updated information (which is what this thread is about) it sends a signal saying as much and if a user action requires sending info to the instrument it sends a signal with the information. This means the GUI is completely abstracted (even connection wise) from everything else. When it's done, it's done. Attaching to it means you hook up relevant signals; which a third class handles. (Again, this implementation means the GUI code doesn't need to be touched for any reason other than handling GUI relevant code.)

Similarly, if the instrument tells us (via it's own event system which *is* asynchronous with mine) that its state has changed, the View Model(s) that care about the change will send off signals containing the data to relevant GUI objects. These "View Models" are essentially wrapped functionality of the instrument changing it's COM style interfaces into an EDP Qt implementation. It also allows us to break these very large classes into much more manageable chunks (similar to what interfaces might do for you) but the chunks are modular and can be added/removed on a whim without breaking the system.

[wysota]

In order to update via the first style one follows the logic I put forth earlier; meaning that the object that needs information receives it via a slot. This slot is not in the "local" scope and thus can't utilize your locals, so it would have to save to members (or some other class-wide variable.) This is what I mean.

I don't know why you are assuming that accepting some calculation result must ultimately change some variable (be it local or global).

I don't think it's too complex :S

I think it is complex and error prone. You are falsely assuming that nothing can change (more than once) the values of m_min, m_max and m_default between lines 3 and 4 of your last snippet. Why don't you just accept values in the slot that is to update the UI in a form of a "state" object (like QNetworkReply does)?

By the way, having something that has "1000s of functions" is probably a good place to start refactoring your code. Strategy or Visitor design patterns might be good approaches to take.

[amleto]

If your problem is an initialisation on - it looks like it is as some class doesn't have enough information to be formed correctly in your current design, then perhaps you may want to think of some factory that can take care of things a bit better.

Also, I abhor code such as

X* x = new X;
// a few lines of code
delete x;

X* x = new X;
// a few lines of code
delete x;

To copy to clipboard, switch view to plain text mode 

really, what is the point of the heap allocation?

X x; // DONE!

X x; // DONE!

To copy to clipboard, switch view to plain text mode 

Better!

If the GUI needs updated information (which is what this thread is about) it sends a signal saying as much

Why/How does the gui know it needs updating? Surely that is the whole point of an event that should be *telling* the gui to update.

[tescrin]

I don't know why you are assuming that accepting some calculation result must ultimately change some variable (be it local or global).

I think it is complex and error prone. You are falsely assuming that nothing can change (more than once) the values of m_min, m_max and m_default between lines 3 and 4 of your last snippet. Why don't you just accept values in the slot that is to update the UI in a form of a "state" object (like QNetworkReply does)?

By the way, having something that has "1000s of functions" is probably a good place to start refactoring your code. Strategy or Visitor design patterns might be good approaches to take.

I apologize for being overtly aggressive; but you're being a bit condescending here. I know full well how I'd go about refactoring the thing and that's on someone else's horizon. That is not, however, my project. Waiting or pushing for this is also not an option. I can't mess with the code base below the GUI. If this were testing I'd be "Grey Box testing."

Were you to ask a Windows-7 (or w/e) question would you expect an answer telling you to go manipulate the kernal? If so, you have no need of enemies


To your other points:
-You said in the other thread that Signals/Slots were synchronous; I.E. if the signal is sent it's akin to direct function calls *unless specifically asynchronous.* If that's the case then this is deterministic and we can assume the value was updated; just like with a direct function call. What am I missing about Signals that makes this incorrect? (Are you assuming I'm still planning on using references?)

-I apologize, but reading QNetworkReply hasn't given me insights on how it applies to my situation. I have to query; I do not have the luxury of being handed data. Directly querying isn't an option if I want the GUI to be separate from the instrument's interface.

-The system itself isn't terrible complex. This goofy problem may be, but the system itself is about as simple as it can get until the instrument can be overhauled. (Something that talks to a specific part of the instrument, can be hooked up however you want, and something that displays the data, hooked up to whatever it cares about, neither of them ever know of each other. Each object can be tested in isolation, Loosely coupled, the communication is only 2 steps in either direction (GUI->model->instrument, instrument->model->GUI), modular. It's not complex by any stretch.


@amleto
It's a design constraint. I'd store the mins/maxs/etc as constants that are accessible as is; but due to the instrument I must probe to see what my maxs/mins are (and do it in real time.) The keypad I've built is generic, so it can't be expected to hook up through some com system utilizing specific interfaces, etc. to retrieve it's values; in part because their system is not polymorphic (at least, not in a way that's utilizable for anything but grabbing other interfaces) and in part because that's placing *a lot* of responsibility and knowledge on a lowly keypad.

What I mean by "if the GUI needs updated information" is such as in this case, where I have to probe the instrument to figure out what the current allowed values are. I'm not saying their design is great, but it's what I have to work with. So as the GUI I say "hey I need to know what this value is now before I hand the user this keypad" and then I pass it up. A factory would be fine, but again, they don't have a system with lots of objects and overloaded functions to nicely attach to; everything is specific.

Also, I'd note that their COM system didn't include any Event-firing functions for these values; so I have no way of detecting whether or not the value has changed without querying the instrument. Further, timer options (to poll the instrument) would be invalid as that's just a race condition in disguise.

I apologize, but I don't know why you bring up heap vs. stack allocation.

[wysota]

-You said in the other thread that Signals/Slots were synchronous

Only within a single thread and only by default. I presume you currently do have just one thread here but a month from now someone doing some clever refactoring elsewhere in the code might decide to start using threads and your code will magically stop working without you even knowing that.

-I apologize, but reading QNetworkReply hasn't given me insights on how it applies to my situation. I have to query; I do not have the luxury of being handed data. Directly querying isn't an option if I want the GUI to be separate from the instrument's interface.

It's exactly the same with networking. You query the server for data (passing QNetworkRequest object to QNetworkAccessManager) and some time later you are given an answer to it when QNAM reads data from the socket and signals the QNetworkReply object it gave you earlier. In other words there is a conversation going on:

YOU: "Hey, give me the data I need, please"
SERVICE: "Ok, no problem. Here is the reply object, I'll let you know when it's ready"
... time passes on ...
SERVICE: "Hey man, the object I gave you earlier contains your data now, have fun with it"
YOU: "Thanks bro, you're the best, now I can read the data from the object you gave me".

Get the picture? No member variables, no references, just an external data object one side can write to and the other can read from and a signal in the data object saying "hey, I have updated data for you".

[amleto]

From what you have said, a factory would help. The keyboard would be a keyboard only, none of these signal + slots needed just for initialisation data. All that rigmarole can be abstracted away in a factory. Depending on your differing instrument or keyboard usage, merely create the keyboard directly for the trivial case, or use a different concrete factory.


As for heap vs stack, if you don't know why one is very frowned upon in such examples, I suggest you do some further reading

[tescrin]

I think I get it now. Currently I have specifics, and you (wysota) are implying that I should do this with a generic object *here*; that way it stores the variables in a generic fashion and can be called upon. I had thought of something similar for below the GUI, but hadn't thought of applying that mode of thought to this.

I'll attempt to run with that idea; many thanks!




@amleto
I understand what you meant in heap vs. stack, I just wasn't sure what triggered the conversation. IMO, the only reasons to use pointers in said situation is:
-You're unsure of the lifetime of the object (Which is a bad excuse as you haven't put forethought into the design.)
-Consistency in code. (always "->" instead of a mix of "->" and ".". Not sure if this counts as a good argument lol)

Just for you; I'll keep an extra close eye on my memory usage.


Thank you both for the assistance. I know I bring up backwards or complicated questions somewhat frequently, so I appreciate the help on something I otherwise don't have a lot of resources for.



EDIT:
http://www.developer.nokia.com/Commu...a_Signal_in_Qt

Seeing it in code helps me get how you do it; but I feel like it has the same issue as the reference implementation doesn't it? I guess if it had multiple slots it would end up saying "readyRead()" multiple times in this case, which isn't a problem. *stewing*

EDIT2:
I dabbled with some psuedo-code:

connect(keypadX_option_was_clicked, ready_keypadX())
 
ready_keypadX(){
  transporter* Jason_Statham;
  connect(Jason_Statham->isReady(), create_keypadX()
  emit(I_need_an_update(Jason_Statham));
}
 
create_keypadX(){
  //cast sender() as_transporter()
  //get data from Jason
}

connect(keypadX_option_was_clicked, ready_keypadX())

ready_keypadX(){
  transporter* Jason_Statham;
  connect(Jason_Statham->isReady(), create_keypadX()
  emit(I_need_an_update(Jason_Statham));
}

create_keypadX(){
  //cast sender() as_transporter()
  //get data from Jason
}

To copy to clipboard, switch view to plain text mode 

Please correct me if I'm wrong. That said, I'm still curious why this isn't a "multiple slot"-problem still. Jason will have different information at different times if connected to multiple slots. Either way this is a helpful suggestion; I just.. want to be clear about whether the same problem is here or not. It's certainly better than references (we don't allow others to directly manipulate data, we can utilize a mutex, etc..)

[wysota]

-Consistency in code. (always "->" instead of a mix of "->" and ".". Not sure if this counts as a good argument lol)

No, not really

Sometimes there is another reason to use heap for data -- when stack space is limited.

http://www.developer.nokia.com/Commu...a_Signal_in_Qt

Seeing it in code helps me get how you do it; but I feel like it has the same issue as the reference implementation doesn't it? I guess if it had multiple slots it would end up saying "readyRead()" multiple times in this case, which isn't a problem. *stewing*

I wouldn't use that solution in your case. I know it is easier to comprehend code that is synchronous (or at least looks like one) but there are many advantages of asynchronous programming.

[tescrin]

You ninja'd my psuedo-code a little bit. I assume you meant their first code block (when saying synchronous) and not the second example? If you get the chance to support or tear down my psuedo-code/their-second-example, it'd be appreciated.

I think I get it.. I'm just wary.

[amleto]

EDIT2:
I dabbled with some psuedo-code:

connect(keypadX_option_was_clicked, ready_keypadX())
 
ready_keypadX(){
  transporter* Jason_Statham;
  connect(Jason_Statham->isReady(), create_keypadX() // Jason_Statham->isReady() - you just got yourself some 'undefined behaviour'.
  emit(I_need_an_update(Jason_Statham)); // what is the receiver of this dangling pointer meant to do with it?
}
 
create_keypadX(){
  //cast sender() as_transporter()
  //get data from Jason
}

connect(keypadX_option_was_clicked, ready_keypadX())

ready_keypadX(){
  transporter* Jason_Statham;
  connect(Jason_Statham->isReady(), create_keypadX() // Jason_Statham->isReady() - you just got yourself some 'undefined behaviour'.
  emit(I_need_an_update(Jason_Statham)); // what is the receiver of this dangling pointer meant to do with it?
}

create_keypadX(){
  //cast sender() as_transporter()
  //get data from Jason
}

To copy to clipboard, switch view to plain text mode 

Please correct me if I'm wrong. That said, I'm still curious why this isn't a "multiple slot"-problem still. Jason will have different information at different times if connected to multiple slots. Either way this is a helpful suggestion; I just.. want to be clear about whether the same problem is here or not. It's certainly better than references (we don't allow others to directly manipulate data, we can utilize a mutex, etc..)

comments in code, plus: create_keypadX having to use sender() and cast to transport is very poor design - totally rigid and not encapsulated. Now you can only make a keypad by using a 'transporter'.

connect(keypadX_option_was_clicked, ready_keypadX())
 
ready_keypadX(){
  transporter* Jason_Statham = new transporter;
  connect(xxx, isReady(transporter*), this, create_keypadX(transporter*));
  emit(I_need_an_update(Jason_Statham)); // what is the receiver of this dangling pointer meant to do with it?
}
 
create_keypadX(transporter* jason){
  // now I have all the data from jason.
}

connect(keypadX_option_was_clicked, ready_keypadX())

ready_keypadX(){
  transporter* Jason_Statham = new transporter;
  connect(xxx, isReady(transporter*), this, create_keypadX(transporter*));
  emit(I_need_an_update(Jason_Statham)); // what is the receiver of this dangling pointer meant to do with it?
}

create_keypadX(transporter* jason){
  // now I have all the data from jason.
}

To copy to clipboard, switch view to plain text mode 

[wysota]

I assume you meant their first code block (when saying synchronous) and not the second example?

Yes.

If you get the chance to support or tear down my psuedo-code/their-second-example, it'd be appreciated.

I don't see anything to be torn there Looking at your pseudo-code I'd probably let the service create the state object instead of the client (like in your code). You're still using signals there but I don't see any benefits of that compared to calling the service directly.

[tescrin]

On the code comments; it's similar to my first block: it's not meant to compile or work; it's just pseudo-code to get my thoughts out. Obvs Jason needs to be initialized,etc. Besides; Jason_Statham is always ready

However, good point on the rigidness. I'll see if I can't fix that somehow. [At least I have a way forward that isn't inherently broken while I think on this.] Honestly, if I weren't replicating something I'd have the freedom to never have this problem in the first place. lol


Thanks again guys!


@Wysota
I could very well be wrong in my approach; but the benefit is that the GUI has an independent way to update it; requiring no direct calls/references/etc; enabling all of the "wires" to be hooked up in one location with the GUI and View-Model-Object layer being abstracted from each other. Maybe it's not a strong enough reason. I imagine I'll have a proper review of this thing in a month or two when it's polished up a bit. Maybe I'm a revision or two off of what the end-product will be. (I'm basically building a large prototype/proof-of-concept by utilizing only some of the interfaces of the instrument.)

[wysota]

I could very well be wrong in my approach; but the benefit is that the GUI has an independent way to update it;

That would be true if all components would trully be loosely coupled. However in my opinion the presence of the state object I've been convincing you to use which is strongly related to the service which in turn couples the service to the UI.

In my opinion in this situation it is better to implement a regular listener pattern however of course that's mostly a matter of taste. I've been always running into trouble sooner or later when trying to do it like you're trying to do it now and I found what I'm proposing more reliable.

[tescrin]

@amelto
I think I was concerned initially due to having to match signals/slots arguments; but I think it'd be solved easy enough by (building on what you doing):

(again, think psuedo-code; I didn't worry about a lot of the syntax)

class transport{
  void you_are_ready() {emit im_ready(this);}
  signals:
    void im_ready(transport*);
}
 
connect(im_ready(transport*),create_keypad(transport*))

class transport{
  void you_are_ready() {emit im_ready(this);}
  signals:
    void im_ready(transport*);
}

connect(im_ready(transport*),create_keypad(transport*))

To copy to clipboard, switch view to plain text mode 

This does make the dependency on "transporter" much more readable and one can perform testing easier by passing an "empty" (or just uninteresting) transport into it. I was trying to figure out how to pass in an "uninteresting" transport and I guess this is the best way to handle that among its other benefits.



@wysota
The reason I wasn't prepared for this problem was that the design *was* just a listener pattern; but I have a few constraints in the initial version of this project:
-the instrument can't be changed (so the fact it won't alert me to changes in these specific variables is a problem only solved by going into it)
-I'm actually replicating/rebuilding a GUI; so I have to, in some sense, replicate it's problems before I can fix them lol. I know it sounds odd, but I'm dealing with VB6 with a COM backend (this is all from 10+ years ago..); eventually it'll all be golden; but baby-steps for now.

For fun I'll mention that I built a layer in the COM side that monitors all of the traffic (and can test against it.) This is why I know what I *have* to call to get it to work against the old GUI without upsetting things. I can certainly manipulate it work better, but for now I'm "proving" that the tech can be upgraded to new code while preserving all of the details. When this is the actual frontend I imagine there will be opportunity to improve the rest of the instrument before the next-gen version is out; although if I were lucky I'd just get placed on the next-gen version to keep ugliness like this out of it .



Thanks again for all of the input; it's helping eliminate chunks of functions, signals, ugliness and reduced this set of connections by at least half or so. (among getting rid of the potential race condition and overwriting issues.)