8

Problem
I recently read a lot about Singletons being bad and how dependency injection (which I understand as "using interfaces") is better. When I implemented part of this with callbacks/interfaces/DI and adhering to the interface segregation principle, I ended up with quite a mess.

The dependencies of a UI parent where basically those of all its children combined, so the further up the hierarchy a UI element was, the more bloated its constructor was.

All the way on top of the UI hierarchy was an Application class, holding the info about the current selection and a reference to a 3d model which needs to reflect changes. The application class was implementing 8 interfaces, and this was only roundabout a fifth of the products (/ interfaces) to come!

I currently work with a singleton holding the current selection and the UI elements having a function to update themselves. This function trickles down the UI tree and UI elements then access the current selection singleton as needed. The code seems cleaner to me this way.

Question
Is a singleton maybe appropriate for this project?
If not, is there a fundamental flaw in my thinking and/or implementation of DI which makes it so cumbersome?

Additional info about the project
Type: Shopping basket for apartments, with bells and whistles
Size: 2 man-months for code and UI
Maintenance: No running updates, but maybe "version 2.0" later
Environment: Using C# in Unity, which uses an Entity Component system

In almost all cases, user interaction triggers several actions. For example, when the user selects an item

  • the UI part showing that item and its description needs to be updated. For this, it also needs to get some info from a 3d model in order to calculate the price.
  • further up the UI, the overall total price needs to be updated
  • a corresponding function in a class on a 3d model needs to be called in order to display the changes there
R. Schmitz
  • 2,608
  • 1
  • 16
  • 28
  • DI isn't just about using interfaces, it is the ability to swap out concretions which is especially useful in the unit testing sphere... – Robbie Dee Apr 26 '16 at 11:07
  • 1
    Also, I've yet to see a problem where a singleton is the preferred solution. The canonical example people always seem to state is a log writer but even here you may want to write to a number of different logs (error, debug etc). – Robbie Dee Apr 26 '16 at 11:13
  • @RobbieDee Yes, DI is about more, but I can not see a situation where using an interface is not DI. And if a singleton is not the preferred solution - which I also assume - then why is the preferred solution so messy? That is my main question, but I wanted to keep it open, as sometimes the general rule does not apply. – R. Schmitz Apr 26 '16 at 11:42
  • The interface-concretion arrangement is also a feature of mocking frameworks. They've also been in OO languages for a good few decades... – Robbie Dee Apr 26 '16 at 11:45
  • I don't get your point. – R. Schmitz Apr 26 '16 at 12:01
  • Various remarks you make, eg "The application class was implementing 8 interfaces, and this was only roundabout a fifth of the products" suggest that the problem doesn't lie with DI, but with your design. It sounds like your classes are doing too much. Each class should only need to implement 1-2 interfaces to enable DI to glue everything together at run-time. Are any of your classes more than 3-400 lines long? – David Arno Apr 26 '16 at 12:42
  • @R.Schmitz *"...I can not see a situation where using an interface is not DI"* - as I said: mocking frameworks... – Robbie Dee Apr 26 '16 at 12:47
  • @RobbieDee ...which help you testing already injected dependencies. – R. Schmitz Apr 26 '16 at 12:54
  • @DavidArno No, most of the files are under 200 lines even. The issue is more about otherwise isolated classes having to modify some 'global' parts, such as a 3d model and parts of the UI which e.g. calculate the total price. – R. Schmitz Apr 26 '16 at 12:59
  • @R.Schmitz Yes, but you can use mocking frameworks without DI. To cut a long story short, interfaces are used widely outside DI. – Robbie Dee Apr 26 '16 at 14:09
  • @R.Schmitz is there a reason why the 3d model doesn't listen for application-level events raised by the UI and respond to those instead of having some higher level component responsible for changing it? – Peter Smith Apr 26 '16 at 17:12
  • @PeterSmith Wouldn't that amount to the same? As in, I would then have to pass those events instead of the interfaces? – R. Schmitz Apr 28 '16 at 08:00
  • @R.Schmitz the only thing that would be more singleton\global in that aspect would be the list of event listeners, although support for events are built into C# so then it's just a matter of registering the delegates. Typically the Mediator pattern would be used here in a singleton type fashion (either managed as a singleton through DI/IOC or directly a singleton). – Peter Smith Apr 28 '16 at 14:23
  • @PeterSmith That is similar to what I'm doing at the moment - but I'm still using global fields there. If you can give a small explanation of how to keep it testable in this situation, please put it into an answer and I'll mark it as answered. – R. Schmitz Apr 28 '16 at 14:57
  • Why does application needs to depend on the dependencies of its children? – Winston Ewert May 02 '16 at 18:33
  • @Winston It doesn't, but the children depend on several parts of the application (like the database models, the 3d model, the current selection of products etc.) – R. Schmitz May 04 '16 at 10:10
  • Wait, then why are your constructors bloated? – Winston Ewert May 04 '16 at 15:43
  • Because the children need to be given to listeners so they can call back. – R. Schmitz May 06 '16 at 15:51

2 Answers2

4

I think the question is a symptom, not a solution.

I recently read a lot about Singletons being bad and how dependency injection (which I understand as "using interfaces") is better. When I implemented part of this with callbacks/interfaces/DI and adhering to the interface segregation principle, I ended up with quite a mess.

A solution looking for a problem; that and misunderstanding it probably is corrupting your design. Have you read this SO question about DI vs Singleton, different concepts or not? I read that as simply wrapping a singleton so the client does not have to deal with a singleton. It.is.just.good.old.encapsulation. I think that's spot on.

the further up the hierarchy a UI element was, the more bloated its constructor was.

Build the smaller bits first then pass them into the constructor of the thing they belong in and pass that bigger thing into the constructor of the next bigger thing...

If you've got complex construction issues, use the factory or builder pattern. Bottom line there is that complex construction pulled into it's own class to keep other classes neat, clean, comprehensible, etc.

The application class was implementing 8 interfaces, and this was only roundabout a fifth of the products (/ interfaces) to come!

This sounds like the absence of extend-ability. Core design is missing and everything is being crammed in from the top. There should be more bottom up construction, composition, and inheritance going on.

I wonder if your design is "top heavy." Sounds like we're trying to make one class be everything or anything that it could be. And the fact that it is a UI class, not a business domain class, that really makes me wonder about separation of concerns.

Revisit your design from the beginning and be sure there is a solid, basic product abstraction that can be built upon to make more complex or different category productions. Then you better have custom collections of these things so you have somewhere to put "collection level" functionality - like that "3rd model" you mention.

... 3d model which needs to reflect changes.

Much of this may fit in custom collection classes. It may also be independent class structure unto itself due to depth and complexity. These two things are not mutually exclusive.

Read about the visitor pattern. It's the idea of having whole chucks of functionality wired abstractly to different types.

Design and DI

90% of all the dependency injection you will ever do is constructor parameter passing. So says the quy who wrote the book. Design your classes well and avoid polluting that thought process with some vague notion about needing to use a DI container-thingy. If you need it, your design will suggest it to you so to speak.

Focus on modeling the apartment shopping domain.

Avoid the Jessica Simpson approach to design: "I totally don't know what that means, but I want it."

The following are just wrong:

  • I'm supposed to use interfaces
  • I'm not supposed to use a singleton
  • I need DI (whatever that is)
  • I'm supposed to use composition, not inheritance
  • I'm supposed to avoid inheritance
  • I need to use patterns
Community
  • 1
radarbob
  • 5,808
  • 18
  • 31
  • I tried getting rid of all singletons I used and some of the things you said happened more or less automatically. The rest makes a lot of sense, too and I'm gonna take your advice and read up on the visitor pattern etc. All in all a well written answer, thank you! – R. Schmitz May 04 '16 at 09:48
  • Just one point, and I'm not trying to be a help vampire here, but it was _kind of_ part of the original question: The general consensus (and it is based on valid reasons) seems to be that you are indeed not supposed to use a singleton. You write "'I'm not supposed to use a singleton' is wrong" - in which situation would it be appropriate to use one then? – R. Schmitz May 04 '16 at 09:56
  • All I'm saying is, "it depends." Too often I see maxims taken quite literally. – radarbob May 04 '16 at 14:36
3

"Class heirarchy" is a bit of a red flag. Hypothetical: a web page with 5 widgets. The page is not an ancestor of any of the widgets. It may hold a reference to those widgets. But it is not an ancestor. Instead of class heirarchy, consider using composition. Each of the 5 widgets can be constructed on its own, without reference to the other widgets or the top page. The top page is then constructed with enough information to build a basic page and layout the widget objects(Collection) that are passed to it. The page is responsible for layout, etc, but not for the construction and logic of the widgets.

Once you use composition, DI is your best friend. DI allows you swap out each widget for whatever version or widget type you define in DI. Construction of the widgets is captured in the DI and is separate from the top page. Perhaps even the composition of the Collection can be defined in your DI. The top page performs layout based on the widgets passed to it, as it should. No changes to the top page constructor needed. Top page only needs the logic to perform layout of the widgets and pass info to/from widget based on the defined interfaces.

Instead of passing listeners up and down the chain of composition, inject a collection of listeners to those widgets that need it. Inject the collection into a publisher so they can publish to the collection. Inject the collection in the listeners so they can add themselves to the collection. The collection cuts across all the objects in the chain of composition.

Robert Harvey
  • 198,589
  • 55
  • 464
  • 673
scorpdaddy
  • 206
  • 1
  • 3
  • Sorry, "class hierarchy" was wrong wording here; it actually wasn't class hierarchy, but rather UI hierarchy. The 'widgets' are not subclasses of the application class, but the 'page' holds references to them in order to publish UI updates. It was very testable with this structure, but there was also a lot of overhead especially in the constructors, just passing on a lot of listeners for their (UI) children. – R. Schmitz Apr 26 '16 at 14:35
  • @R.Schmitz: Did the overhead matter? Was the UI sluggish, and was the design you described to blame? – Robert Harvey Apr 26 '16 at 14:57
  • @R.Schmitz Can you elaborate on "passing on a lot of listeners"? Maybe my experience with DI in C# is low, but something tells me that wouldn't be necessary (at least in the constructor) with proper design. – Katana314 Apr 26 '16 at 15:14
  • @RobertHarvey No performance loss at all, it's only about readability. – R. Schmitz Apr 26 '16 at 15:55
  • @Katana314 For example, when an item is added, the 3dmodel needs to be updated and it does not belong in any of the product categories, so it is referenced in the application class, which listens to items being added/removed/changed. In the end that would pretty much be the same for every product category. Other UI parts also react (and listen), but the message needs to travel to the top because that's where the 3dmodel reference and the actual data (which is then also updated) is. – R. Schmitz Apr 26 '16 at 16:04
  • @R.Schmitz And so as I understand it, in `ChildOfChildOfChild`, you might have a method that calls `applicationInstance.OnItemAdded(item)`? That could be something you could reorganize so that a child arbitrarily notifies "all 0-n listeners". You might have events that are broadcast to many areas that don't need them, but as long as listeners are easy to add, that's the main thing I think. – Katana314 Apr 26 '16 at 16:55
  • @Katana314 That sounds like a violation of the interface segregation principle to me, because I would have classes implementing several interface methods they do not need. Of course it might be that I'm following SOLID to religiously here. – R. Schmitz Apr 28 '16 at 08:05
  • @R.Schmitz I certainly wasn't thinking of adding egregious interfaces to classes that won't need them. There are multiple event-handling systems I've dealt with that wouldn't have issues communicating from chidren to parents, ideally with no actual upwards referencing; but it's too much to describe in a comment. – Katana314 Apr 28 '16 at 20:46
  • @Katana314 OK, well if you write an answer "use eventsystems" with a small explanation why that would remain testable, I'll mark it as answered. – R. Schmitz Apr 29 '16 at 09:08
  • Would a DI Collection of "listeners for x" help. Widgets could access the collection and add themselves as listeners for x. Publishers could access the DI collection and publish to the collection. Instead of "passing" listeners - inject them. – scorpdaddy Apr 30 '16 at 01:24
  • @scorpdaddy That sounds like the event system, just without events and with listeners instead. Well, that would also work. If you care to write that answer, I'll mark that as answered instead. Otherwise I'm gonna answer it myself tomorrow. – R. Schmitz May 03 '16 at 08:03