-5
\$\begingroup\$

I am trying to fill a hashtable with LicenseID and FullName when user first logs in. I don't have access to the Licenses database right now. So "Context" is a place holder. I am at loss if what I am doing is kosher.

On load of first MVC C# View:

MySingleton.GetInstance().BuildAllGrowers();

Here is MySingleton class

   public class MySingleton
        {
       private static Hashtable allGrowers = new Hashtable();

        public Hashtable Singleton
            {
            get
                {
                return allGrowers;
                }
            }

        // Class implementation here..
        public void BuildAllGrowers()
            {

            //Retrieve all Grower Names from medical marijuana database
            //into allGrowers
            var licenses = context.Licenses;

            foreach (var licenses in licenses)
                {
                //Fill Hastable
                allGrowers.Add(licenses.id, licenses.FullName);
                }
            }
       }
\$\endgroup\$

closed as off-topic by Phrancis, Vogel612, Edward, Pimgd, syb0rg May 30 '16 at 23:21

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ Your code snippet doesn't match the implementation you're showing. Is your singleton accessible through the Singleton property or the GetInstance method? They're two different implementations. \$\endgroup\$ – Avner Shahar-Kashtan May 30 '16 at 18:44
  • \$\begingroup\$ Oh, so your saying I take out the "GetInstance()"? Like this: MySingleton.BuildAllGrowers(); ?? I was assuming calling MySingleton would fill the hashtable somewhere in memory so it could be used for other Views. \$\endgroup\$ – JustJohn May 30 '16 at 18:56
  • \$\begingroup\$ No, that won't work - or compile. But your code calls a GetInstance method, while your class has a Singleton property. Pick one. \$\endgroup\$ – Avner Shahar-Kashtan May 30 '16 at 19:02
  • 3
    \$\begingroup\$ Welcome to Code Review! I have rolled back the last edit. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Vogel612 May 30 '16 at 19:47
  • 2
    \$\begingroup\$ I imagine you're getting downvotes because it seems the question was constructed without care. The indentation of the code is crap, the question content is crap (you're asking for a review on code which is a stub implementation) and whilst it's great that you're learning things, this really isn't the place for this kind of code. Stub code is off-topic. \$\endgroup\$ – Pimgd May 30 '16 at 22:12
2
\$\begingroup\$

I'm not going to go into the whole question of whether you need a singleton at all or not. I feel that's out of scope. However, if you feel the need for a singleton, there are two issues around it: the first is instantiation, the second is thread-safe access.

Instantiation

As far as singleton instantiation goes, your implementation is a good one. My go-to reference for singletons in C# is this article which outlines different implementation. The one you've chosen - a static field which is instantiated the first time your type is accessed, in a thread-safe fashion, and retrieved though a read-only property, is a good enough implementation.

Thread-safe Access

However, even if the singleton is instantiated once, calls to it are not thread-safe. If two threads call MySingleton.Singleton.BuildAllGrowers at once, you'll have collisions - both threads will try to add to the Hashtable at once, and the most likely result is an exception when you try to Add() an item that's already been added.

If your aim is to have a hashtable that's lazy-loaded from the DB the first time it's accessed, your best bet is using the fifth method in the article above (using the nested class), and having the BuildAllGrowers method in the nested class's static constructor. That way it will happen once, before the first caller can access it.

Hashtable?!

In addition to the issue of the Singleton, I really have to ask - Hashtable?! The Hashtable class has been deprecated since 2005, and should never be used in projects based on .NET 2.0 or newer, which MVC certainly is. You should use Dictionary<K,V>, which in your case would probably be Dictionary<string,string> or Dictionary<int,string> - they're type-safe (which Hashtable is not) and just as efficient.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.