I have no code here, as this is more of a design question (I assume this is still the best place to ask it).

I have a very simple server in java which stores a mapping between certain values and UUID which are to be used by many systems across multiple platforms. It accepts a connection from a client and creates a clientSocket which stores the socket and all the other relevant data unique to that connection. Each clientSocket will run in their own thread and will block on the socket waiting for a read. I expect very little strain on this system, it will rarely get called, but when it does get a call it will need to respond quickly and due to the risk of it having a peak time with multiple calls coming in at once threaded is still better.

Each thread has a reference to a Mapper class which stores the mapping of UUID which it's reporting to others (with proper synchronization of course). This all works until I have to add a new UUID to the list. When this happens I want to report to all clients that care about that particular UUID that a new one was added. I can't multicast (limitation of the system I'm running on) so I'm having each socket send the message to the client through the established socket. However, since each thread only knows about the socket it's waiting on I didn't have a clear method of looking up every thread/socket that cares about the data to inform them of the new UUID.

Polling is out mostly because it seems a little too convoluted to try to maintain a list of newly added UUID. My solution as of now is to have the 'parent' class which creates the mapper class and spawns all the threads pass itself as an argument to the mapper. Then when the mapper creates a new UUID it can make a call to the parent class telling it to send out updates to all the other sockets that care about the change.

I'm concerned that this may be a bad design due to the use of a circular reference; parent has a reference to mapper (to pass it to new ClientSocket threads) and mapper points to parent. It doesn't really feel like a bad design to me but I wanted to check since circular references are suppose to be bad.

Note: I realize this means that the thread associated with whatever socket originally received the request that spawned the creation of a UUID is going to pay the 'cost' of outputting to all the other clients that care about the new UUID. I don't care about this; as I said I suspect the client to receive only intermittent messages. It's unlikely for one socket to receive multiple messages at one time, and there won't be that many sockets so it shouldn't take too long to send messages to each of them. Perhaps later I'll fix the fact that I'm saddling higher work load on whatever unfortunate thread gets the first request; but for now I think it's fine.

migrated from codereview.stackexchange.com Sep 6 '12 at 17:50

This question came from our site for peer programmer code reviews.


A general way these references are broken is via interfaces. Instead of the parent class having a reference to the mapper, have it hold a reference to IWhateverParentNeedsFromMapper. Instead of the mapper having a reference to the parent, have it hold a reference to IWhateverMapperNeedsFromParent.

Often, thinking about the problem in this way ends up with a nice descriptive interface name, and a small set of operations within it.

You don't break the "real" circular reference, but instead that reference is an implementation detail instead of a hard requirement of your class design.

As a side note, this:

I wanted to check since circular references are suppose to be bad

Don't think like that. :) There are lots and lots of things that are supposed to be bad powering everything you use every day. Working code trumps "theoretically better" code under most circumstances.

  • 3
    +1, but allow me a comment on "You don't break the real circular reference" - actually, you move it from compile time to run time - which is a good thing, since it will allow to create mapper instances without "real" parents, one can provide a mock instance of "IWhateverMapperNeedsFromParent" easily to allow the creation of unit tests for Mapper. My point is - breaking the dependencies is really useful, it's not just "for the book". – Doc Brown Sep 6 '12 at 18:44
  • Very true, the testing benefits of using interfaces at boundaries like this should not be overlooked! – Nick Veys Sep 6 '12 at 20:23
  • Pretty much all testing frameworks can create mocks without interfaces. Creating interfaces has its place, but it's over-used and creates a lot of over-complexity in design and un-needed code. – MebAlone Sep 6 '12 at 20:27
  • Thank you for your reply. I won't be using interfaces in this case due to the absurd simlicity of the code; it really feels like overkill. However, I will keep the idea in mind for future situations like this. Since my question was more to learn other design approches in the first place (the code is really basic enough that anyone will be able to understand it no matter what design I select) I consider your answer to still be quite valuable. – dsollen Sep 6 '12 at 21:39

It isn't a bad design. Back pointers are a commonly used mechanism. There's already a dependency between the classes, so you're not creating a coupling where none existing. You could make it an abstract coupling by introducing interfaces, IF you choose to, but that violates YAGNI. Incidentally, note that the java import statement supports circular dependencies and Spring setter injection enables circular dependencies by supporting two stage construction.

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.