Singleton pattern... the wrong way.



  • I just found this gem in the codebase which I inherited:

    internal static Repository FrobnicatorRepo {
        get
        {
            return new Repository<Frobnicator>().Instance;
        }
    }
    

    The underlying code is worse... every time you call that .Instance, it does a shitload of heavy lifting to get a certain object from the underlying datastore. The object isn't cached or kept alive, at all.


  • Discourse touched me in a no-no place

    It's the Simpleton pattern?


  • Winner of the 2016 Presidential Election

    @AlexMedia
    There is no right way of implementing the singleton pattern.


  • Discourse touched me in a no-no place

    @asdf But this one is the very wrong way.


  • sockdevs

    @asdf That depends on if you approve of the use of singletons or not, I guess.

    Doing a singleton right isn't hard:

    //C# 6
    public class Singleton
    {
        private Singleton() {}
    
        public static Singleton Instance { get; } = new Singleton();
    }
    

    But in most cases, a static class would probably be more appropriate.


  • Discourse touched me in a no-no place

    @RaceProUK said in Singleton pattern... the wrong way.:

    Doing a singleton right isn't hard:

    The big problem is whether the singleton is there because of the highlander principle, or whether it's just because this application only happens to need one instance. Singletons are appropriate in the former case, but that's actually pretty rare; most cases that use singletons do so because of convenience only, and that's where everything is messy because of the baked in duff assumptions.


  • Winner of the 2016 Presidential Election

    @dkf And even if you think there can only be one in the application, this assumption might not be true in the future, and then you'll get to rewrite half of your code. Which is what's happening to our code base right now.


  • sockdevs

    @dkf Is there a situation where a singleton would make more sense than a static class?



  • @RaceProUK Sure. Discourse.

    There should be one, and only one, instance of Discourse. And that one should only be kept alive so we can laugh at it.



  • @dkf I recently dabbled with C# UWP coding again, and it's surprisingly difficult to avoid singletons, even just for the DI container. UWP pages and views use their own lifetime management, but you have zero options to inject anything through that. Singletons are the only escape hatch widely available in all parts of your code.



  • How do you mock singletons in tests?



  • @RaceProUK A "singleton class" is an oxymoron in itself.

    Class: a set or category of things having some property or attribute in common and differentiated from others by kind, type, or quality.


  • sockdevs

    @LB_ said in Singleton pattern... the wrong way.:

    How do you mock singletons in tests?

    I point at them and go "Ha ha!" :D

    In all seriousness though, I... don't actually know: I tend to avoid writing singletons.


  • Discourse touched me in a no-no place

    @asdf said in Singleton pattern... the wrong way.:

    And even if you think there can only be one in the application, this assumption might not be true in the future, and then you'll get to rewrite half of your code.

    I've had to do that in the past when converting code from something which was very singleton-heavy into something that would work with OSGi, where instead those singletons were just services which happened to only have one implementation posted to the main workspace. The conversion work was ugly, especially as the code previously used its own weird home-grown scheme that tried to do exactly that but failed miserably and instead just baked in the singletonness all over.


  • mod

    @LB_ You mock out the .Instance() method so it returns a fake object. That's simple enough.

    It's global variables you can't mock out. As long as there's a method to fetch the instance, you're fine.



  • @Yamikuronue said in Singleton pattern... the wrong way.:

    @LB_ You mock out the .Instance() method so it returns a fake object. That's simple enough.

    It's global variables you can't mock out. As long as there's a method to fetch the instance, you're fine.

    As a matter of professional interest, before sounding like a snarky brat, is there a straight forward way of mocking a singleton ala @RaceProUK : https://what.thedailywtf.com/post/1081671

    I work in java but if I'm reading it correctly it's a static instance with static assessors. For these I usually find myself falling back on dependency injection or wrapping the call in a protected method to be exposed to a test double. It's not feasible to remove the singleton at the moment and its very heavy weight.

    I sometimes wonder if toying with classloaders might be a potential answer...

    @dkf said in Singleton pattern... the wrong way.:

    converting code from something which was very singleton-heavy into something that would work with OSGi

    You have my sympathies. I find it highly unlikely any good came of this.



  • @cartman82 There's no way that's true. MEF2 definitely works in UWP. You can make it use shared instances if you must, but there's definitely no need for a statically-accessed-enforced-single-instance in UWP.


  • Winner of the 2016 Presidential Election

    @DogsB said in Singleton pattern... the wrong way.:

    I sometimes wonder if toying with classloaders might be a potential answer...

    See also: https://github.com/powermock/powermock/blob/master/README.md



  • @anonymous234 said in Singleton pattern... the wrong way.:

    Class: a set or category of things having some property or attribute in common and differentiated from others by kind, type, or quality.

    Nowhere in the definition of set (I'm not certain about categories) does it specify a lower limit on the number of elements. Indeed, the empty set is a valid and well-defined set. And clearly, if there is only one element in a set it has all properties and attributes in common with every element.



  • @Magus said in Singleton pattern... the wrong way.:

    @cartman82 There's no way that's true. MEF2 definitely works in UWP. You can make it use shared instances if you must, but there's definitely no need for a statically-accessed-enforced-single-instance in UWP.

    Doesn't look too promising.

    And how would you even access your MEF container or wherever you go to get your services from a page? You'd have to go singleton.

    I've looked at at least 10 UWP examples and they all had singletons at the end of the rainbow.



  • I'm using Unity/Prism and I didn't need a singleton yet.



  • @cartman82 :wtf: do you not know what DI is?

    You never access the container from a page. You make a property, ask for either the shared or a non-shared instance with an attribute, and when the class is created, it will have that instance.

    [Export]
    public SomeClass
    {
      [Import]
      public OtherClass Service { get; set; }
    }
    

    Will cause the composer to create that class with the property set to a non-shared instance, for instance. You can do it with the constructors instead, which is what I prefer, but this was faster to type.

    EDIT: And I'm pretty sure you just have to add SharedAttribute to the thing you're importing to make it use a single instance.



  • @Magus

    I have a disliking for that particular way of doing DI, I highly prefer constructor based dependency injection.



  • @AlexMedia I agree with you. As I said, it was just faster to type!

    Because of course you can also:

    [Export]
    public class SomeClass
    {
      private readonly OtherClass service;
      
      [ImportingConstructor]
      public SomeClass(OtherClass service)
      {
        this.service = service;
      }
    }
    

    in which case the importing constructor assumes an ImportAttribute on all parameters (Though one might be more explicit if they need different sorts of imports)



  • @Magus

    Worst I have ever seen is property based DI on properties that are private. Externally you can no longer see the dependencies of a certain class and it's impossible to get a new instance without going through the IoC container :slight_frown:



  • @Magus I've never seen an UWP app use that style of DI. Are attributes even available in .NET core?

    Most of the DI patterns I've seen are pages calling static container to resolve their dependencies in constructor.



  • @cartman82 The only UWP app I have ever written didn't go much beyond "Hello, world" so I wouldn't know.

    I mostly write ASP.NET MVC or WebAPI stuff, where you can register a DependencyResolver which does the magic for you.


  • sockdevs

    @cartman82 said in Singleton pattern... the wrong way.:

    Are attributes even available in .NET core?

    Yes: ASP.NET Core makes extensive use of them for things like action filters and stuff.


  • Winner of the 2016 Presidential Election

    @AlexMedia said in Singleton pattern... the wrong way.:

    I have a disliking for that particular way of doing DI, I highly prefer constructor based dependency injection.

    +100

    DI is about making all external dependencies explicit. And the only good way of documenting the dependencies of a class is making them required arguments of the constructor.

    Bonus: This also forces you to design sane abstractions, because a 10-argument constructor is obviously wrong, even to incompetent programmers.



  • @cartman82 My game uses this, and is a UWP app. It's just that MEF isn't very well known: it's certainly very good.



  • @asdf said in Singleton pattern... the wrong way.:

    A 10-argument constructor is obviously wrong, even to incompetent programmers.

    Not that obvious, in my experience. I didn't actually count but I'm fairly sure it was upwards of 20. Which were all a handful of types so to figure out which argument was which was not often trivial.

    This was a DTO that did nothing but store a bunch of attributes, so in fairness it did need to have all those things individually set, but surely that's a case for using setters.



  • @CarrieVS

    It depends. If all properties are essential for the construction of an object, the constructor is a perfect place for that.

    Maybe some properties can be grouped together into individual and reusable structures, e.g. an AddressDto instead of having a bunch of address related properties on an object?



  • @AlexMedia They aren't all essential, the object makes sense and is in a usable state with pretty much any combination of them set, and many of them would be null in many cases because some of the data fields were optional.

    We probably could have organised it a bit more (I was myself inexperienced) but given that we didn't, however much better it is in general to set attributes in the constructor, almost any alternative is less bad than a constructor with (approximate values) eight ints, six Strings, seven BigDecimals and a scattering of other things. If you can't look at the constructor and see which argument is which variable without having to count a dozen places in, any other remotely sane way will do.



  • @CarrieVS

    In that case I agree with you, you'd be better off with an empty constructor and a bunch of getters/setters.



  • @asdf said in Singleton pattern... the wrong way.:

    Bonus: This also forces you to design sane abstractions, because a 10-argument constructor is obviously wrong, even to incompetent programmers.

    Of course, then the ten nicely ravioli'd dependencies get refactored into a single God object. Or, for bonus points, into wrappers that do nothing but group them together, making sure the actual complexity is well hidden.

    We have quite a bunch of creeped-up constructors - mostly stemming from our DB layer using per-entity generic repositories, while our business layer is... pretty detached from the underlying model, to put it lightly. There's a lot of stuff that requires us to scour half the database for the data to be displayed, and navigation properties only work when there are actual FK relationships in the database to begin with...


  • Discourse touched me in a no-no place

    @DogsB said in Singleton pattern... the wrong way.:

    You have my sympathies. I find it highly unlikely any good came of this.

    Actually, it improved things quite a bit.

    …

    Yeah, they were pretty messed up before that.


  • Winner of the 2016 Presidential Election

    @Maciejasjmj said in Singleton pattern... the wrong way.:

    Of course, then the ten nicely ravioli'd dependencies get refactored into a single God object.

    In the best case, those God objects have "facade" in their name and actually represent the public interface of an internal module.


  • Discourse touched me in a no-no place

    @asdf said in Singleton pattern... the wrong way.:

    In the best case, those God objects have "facade" in their name and actually represent the public interface of an internal module.

    Those are only Demigod Objects.


  • Winner of the 2016 Presidential Election

    @dkf said in Singleton pattern... the wrong way.:

    @asdf said in Singleton pattern... the wrong way.:

    In the best case, those God objects have "facade" in their name and actually represent the public interface of an internal module.

    Those are only Demigod Objects.

    I hear some of them can be quite herculean.



  • @Dreikin said in Singleton pattern... the wrong way.:

    @dkf said in Singleton pattern... the wrong way.:

    @asdf said in Singleton pattern... the wrong way.:

    In the best case, those God objects have "facade" in their name and actually represent the public interface of an internal module.

    Those are only Demigod Objects.

    I hear some of them can be quite herculean.

    Some could even be titanic in many ways.


  • Winner of the 2016 Presidential Election

    @djls45 said in Singleton pattern... the wrong way.:

    @Dreikin said in Singleton pattern... the wrong way.:

    @dkf said in Singleton pattern... the wrong way.:

    @asdf said in Singleton pattern... the wrong way.:

    In the best case, those God objects have "facade" in their name and actually represent the public interface of an internal module.

    Those are only Demigod Objects.

    I hear some of them can be quite herculean.

    Some could even be titanic in many ways.

    Doomed to failure?


    Filed under:: It is only now I realize how apt the name "Titanic" was, Especially given one of the overthrowers was the god of the seas



  • @RaceProUK said in Singleton pattern... the wrong way.:

    @dkf Is there a situation where a singleton would make more sense than a static class?

    Dispose?

    Rather than hoping someone remembers to call your "Cleanup" method.

    I mean, it's easy to assume you're releasing resources when your process closes, but what if those resources are remote? You could end up leaving a remote resource holding a token to your dead process.



  • @Magus said in Singleton pattern... the wrong way.:

    do you not know what DI is?

    If it's based on IoC, how are you not using singletons. Seems to me you're just masking it.

    Of course, it gives you a layer to mock your container, but if you're accessing services on demand, how do you make it arbitrary, and yet make it not give you back the only single instance of the container?

    There is a singleton somewhere, even if your service is a directory to services...



  • @asdf said in Singleton pattern... the wrong way.:

    @DogsB said in Singleton pattern... the wrong way.:

    I sometimes wonder if toying with classloaders might be a potential answer...

    See also: https://github.com/powermock/powermock/blob/master/README.md

    I toyed with that for a while but it falls into the same trap that Mockito itself falls into. Test code tends to become incomprehensible gibberish eventually that takes more time and effort to maintain. This appears to be twice as bad with powermock.

    That and getting it into a p2 site prove more that my sanity could cope with.


  • Discourse touched me in a no-no place

    @xaade said in Singleton pattern... the wrong way.:

    There is a singleton somewhere, even if your service is a directory to services...

    Yes… but your code shouldn't talk directly to the directory. Or at least as much as possible of your code shouldn't. Instead, you should be given an object that provides access to some service and you should just use it at the right time, without regard for whether it is a singleton or not. True singleton-ness is then corralled at the top level of the application (usually in a single place too) and the rest of the code just does its stuff in its own little world. Where there's a need to do a directory lookup outside of the application configuration phase, you have a class/object that does just that for a specific type of search, and everything that otherwise needs that lookup just delegates.

    DI takes discipline (but not huge amounts of it) to do, and makes the app less crazy. But don't look beneath the covers of the engine that you use to provide DI services; those tend to be pretty nasty. :)


  • Winner of the 2016 Presidential Election

    @xaade
    There are three big problems with Singletons:

    • They're hard to mock.
    • Since you access them via a static method, dependent classes usually hide the dependency on the singleton and its global state. This makes your application more convoluted and harder to maintain/refactor.
    • The class itself and its interface "know" that there can only ever be one instance. Which is why you have to change huge amounts of code if that ever changes.

    The last point is what you seem to be missing. If there is some other object (container) that ensures only one instance of the class is ever created, you're only hard-coding the assumption "there can only ever be one" in a single class instead of everywhere in your application. I would say it's pretty obvious that this is preferable.



  • @RaceProUK Can you do that in C# now! Fuck I am still doing everything .NET 2 stylee.

    I do it the Java way as I learned Java before C# (back when they were basically the same language).


  • sockdevs

    @lucas1 I also learned Java before C#. However, I then kept up with how C# developed :P



  • @RaceProUK when you work mostly with the .NET 2 CLR


  • sockdevs

    @lucas1 Ah, I work with .NET 4.6.2.


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.