0

Stack overflow suggested that this question my be better suited here. I normally work in c#, but I'm working in a vb.net net application currently. So feel free to respond with vb.Net or c#...

Essentially I have a singleton that does all of the heavy lifting of getting global settings which are stored in the database; but there is an interface (dialog form) that allows those settings to change.

So what I did was create a singleton which on the first instantiation (only) runs the public update method. If, however, a user opens the settings dialog, the singleton is instantiated and the update command is invoked separately just in case the user made changes to the settings in the settings dialog... its the only place where I run the Update method outside of the initial instantiation of the singleton object. Conceivably another errant developer could irresponsibly run the Update method over and over again, but it would not harm anything except slow down the application--and this is the main reason I wanted to use a singleton, because there are cases where with calls to the settings is made hundreds if not thousands of times in loops; which would really slow down the application, and also the settings are being called from many random locations depending on what the user does... why don't I use a simple DTO, well, that's described below.

The other crazy thing about this application is that multiple "environments" can be running in the same thread, each with completely different settings (long story). The way the prior developers handled this was by making a call to update a settings DTO every time there was a possible switch or change in the settings or "environment", or upon starting up the application. On a search there are around 40 locations... seems nuts to me. This is on top of the hundreds of calls were the DTO is accessed. I want this singleton to do the heavy lifting and verify which environment the app is in and retrieve the settings only once from the database--except on that one occasion with the settings dialog I mentioned above, where the Update method is invoked.

End result, one nutty singleton... Below is a simplified version of the code. Basically, I'm asking whether is this a reasonable pattern, or is there a better one for this? Note that one potential downside to my solution is that I still need to make a database call, as a check to the current database, every time this class is instantiated. Thanks for the feedback.

   Public Class MySingleton

        Private Shared _Instance As MySingleton = Nothing

        Private _ExampleSetting As String

        Private _CurrentDatabase as DBDatabase

        Public Shared ReadOnly Property GetInstance() As MySingleton
            Get
                If _Instance Is Nothing Then
                      _Instance = New MySingleton()
                ElseIf _CurrentDatabase <> GetCurrentDataBase() Then 
                      _Instance = New MySingleton()
                End If

                Return _Instance
            End Get
        End Property

        Public ReadOnly Property ExampleSetting As String
            Get
                Return _ExampleSetting
            End Get
        End Property


        Private Sub New()

            Update()

        End Sub


        Public Sub Update()

            _CurrentDatabase  = GetCurrentDataBase()

            _ExampleSetting = _CurrentDatabase.GetSetting("exampleSetting")

        End Sub

    End Class
  • That's not a singleton. I'd call it a multiplon. – nvoigt Feb 1 '17 at 16:47
  • 2
    "seems nuts to me" -- they're dynamically updating the state of a thread-global singleton to reflect the current transaction that's being processed in the thread and doing so from multiple locations in the code... that's not just plain nuts, that's daily-WTF-worthy... – Periata Breatta Feb 1 '17 at 16:48
  • @ Periata Breatta I'm going to give dependency injection a try... per my comments in Arno's post below. We'll see what happens – Edward Bagby Feb 1 '17 at 16:53
  • @ Periate Breatta... oh, and I'll only call the singleton when the actual settings need to be retrieved (and after the change settings dialog is closed so I can run the update method) – Edward Bagby Feb 1 '17 at 17:05
6

Your "singleton" isn't a singleton at all. It's a data cache, which can result in different parts of your app having different version of that cache. In C# code, as my VB.Net is weak:

var instance1 = MySingleton.GetInstance();
// assume instance1.ExampleSetting would return "foo"

// do something that changes ExampleSetting to bar, such that
// _CurrentDatabase <> GetCurrentDataBase() is now true

var instance2 = MySingleton.GetInstance(); // new instance
var x = instance1.ExampleSetting; // will be foo
var y = instance2.ExampleSetting; // will be bar

Just because you are overriding the static _Instance with a new instance won't affect references to the previous instances held by other parts of the code. You are relying on the developer calling MySingleton.GetInstance() every time to get the latest "singleton".

Update

To address the question of "is there a better way", one solution is to use dependency injection. Rather than have a globally accessible not-quite-singleton, instead create just one instance of MySingleton and pass it to all parts of the system that need it. Then, whenever it is queried, it can check if the database has changed and update itself, rather than spawning a new instance.

  • That's why I say its an odd singleton. I added the _CurrentDatabase <> GetCurrentDataBase() if/then... but IF the database never changes during a user's session, then it acts as a singleton, no? That's what I want, essentially. What I don't know is whether my pattern is reasonable (even if it is technically not a singleton) or if there is a better approach. Thanks. – Edward Bagby Feb 1 '17 at 15:36
  • Amo... regarding your Update. Do you imagine that would amount to calling the get instance like so: var instance = MySingleton.GetInstance(currentDB); proceeded by creating currentDB: DBDatabase currentDB = GetCurrentDB(); ? Now I'm using c# again, but is that what you mean? Interestin, doing this would not add any more calls to the database than what I had posted above. Would that work, am I missing something? – Edward Bagby Feb 1 '17 at 16:10
  • Again, regarding your update. Back to vb.. the getInstance would look like this: Public Shared ReadOnly Property GetInstance(ByVal currentDB as DBDatabase) As MySingleton... Get... If _Instance Is Nothing Then _Instance = New MySingleton()... If currentDB <> _CurrentDB Then... _CurrentDB = currentDB... Me.Update().... End if... Return _Instance... End Get... End Property – Edward Bagby Feb 1 '17 at 16:25
  • As an edit to the above... instead of passing a database, I'd probably just pass a GUID or some unique identifier for the database. – Edward Bagby Feb 1 '17 at 16:37
1

The essence of a Singleton is that There Can Be Only One. There is ONE instance of a class being created when it's needed the very first time, no other instance is ever created, and the one instance stays alive as long as the app is running. So what you have is not a singleton.

And it is absolutely legal and expected that some bit of code gets a reference to that Singleton once and uses that reference forever. So there is absolutely zero reason to believe that one piece of code will ever call your GetInstance () more than once.

It seems that your GetCurrentDatabase() is some global state that is modified in some uncontrolled way. All you managed to do was turn MySingleton () also into global state that is modified in some uncontrolled way. I'd say you want individual objects, each created by the code that wants to access data in different ways, and these objects can talk to one Singleton that optimises access as well as it can.

1

On closer inspection, as @David Arno has identified,that isnt a singleton. The value returned by GetInstance potentially varies on each call.

Additionally (and this could simply be my lack of knowledge re. VB semantics) the fact that you dont appear to have any restrictions on the use of the constructor for your singleton class means that clients are free to create their own instances.

Singleton has a well understood definition in software development circles. What you have isnt an 'odd singleton', its just not a singleton.

Theres nothing wrong with the internal state of the singleton changing (as long as theres only a single instance of it), so if your individual properties were to check whether or not a database existed, and determine the return value at that point, you could still retain singleton semantics. Whether or not you could make this perform at scale is a different question.

  • 1
    'Private Sub New()' is a private constructor equivalent to 'private ClassName()' in C#/Java – RubberChickenLeader Feb 1 '17 at 15:20
  • Lack of VB chops duly demonstrated... – richzilla Feb 1 '17 at 15:22
  • good old VB6. have not touched it since then though – RubberChickenLeader Feb 1 '17 at 15:23
  • I am checking if _Instance has a value with If _Instance Is Nothing Then in the GetInstance() property. This is an established singleton pattern for VB as far as I'm aware (btw, vb.net is much more advanced than VB6 fortunately). I've just embellished it for the reasons I mentioned. Note my comment to David Amo... which reiterates my main question. Thank you. – Edward Bagby Feb 1 '17 at 15:39
  • 2
    @EdwardBagby If you can have different instances of something existing at the same time, it's not a singleton. That's what people are trying to say. – T. Sar Feb 1 '17 at 15:43

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.