3
\$\begingroup\$

I am looking for advice if the below code is a good idea or not. If not, why? I am building a website which allows a user to purchase items. The user account and cart items will be stored in Session. My approach is to create a singleton class that will manage all the sessions. Is this approach good or bad?

public class SessionFactory
    {
        private static SessionFactory instance;
        private String _UserName;
        private long _CartId { get; set; }

        public Cart UserCart {

            get {

                if (HttpContext.Current.Session["Cart"] != null) {


                    return HttpContext.Current.Session["Cart"] as Cart;
                }

                return new Cart();
            }
            set {

                HttpContext.Current.Session["Cart"] = value;

            }
        }

        private SessionFactory(String username, long cartId)
        {
            this._UserName = username;
            this._CartId = cartId;
            // build Session objects here


        }

        public long CartId{

            get {

                return _CartId;
            }

        }

        public static SessionFactory Instance {

            get {

                if (instance == null) {

                    throw new Exception("Session was not created");
                }
                return instance;
            }

        }

        public static void Create(String userName, long cartId) {

            if (instance != null) {

                throw new Exception("Session Instance already created.");
            }

            instance = new SessionFactory(userName, cartId);
        }
    }
\$\endgroup\$
2
\$\begingroup\$
  • You haven't created a SessionFactory. The name suggests that this object returns session objects. Really, this is just a strongly typed session object. I've done this sort of thing before, and called is "SessionStore" instead. You could name it "ShoppingCartSession" maybe. But SessionFactory is not correct. This is not a factory object.

  • The CartId property and _CartId field: The _CartId private field is unnecessary. You can make the CardId property a public getter, but private setter:

    public long CardId { get; private set; }
    
    ...
    
    private SessionFactory(String username, long cartId)
    {
        this.CartId = cartId;
    
  • Why even have a CartId property? Why not store that on the UserCart object?

    public long CartId { get { return UserCart.Id; } }
    
  • Getters and Setters for Session data: These seem Ok to me, however there is some irrelevant code in the Getters:

    public Cart UserCart {
        get {
    
            if (HttpContext.Current.Session["Cart"] != null) {
    
    
                return HttpContext.Current.Session["Cart"] as Cart;
            }
    
            return new Cart();
        }
        set {
    
            HttpContext.Current.Session["Cart"] = value;
    
        }
    }
    

    The if statement does nothing, since the implicit cast inside the if statement will return a null value if the object in that key is not a Cart object. This can be shortened to:

    public Cart UserCart
    {
        get { return (HttpContext.Current.Session["Cart"] as Cart) ?? new Cart(); }
    
        set { HttpContext.Current.Session["Cart"] = value; }
    }
    

    By using the null-coalescing operator: ??

| improve this answer | |
\$\endgroup\$
5
\$\begingroup\$

There are several remarks I could tell on your singleton implementation but since there's a far more better expert than me on these things, I'll reference his work. His name is Jon Skeet and he has an article on the singleton pattern which can be found here. Here are several points from the article that can help:

Characteristics:

  • A single constructor, which is private and parameterless. This prevents other classes from instantiating it (which would be a violation of the pattern). Note that it also prevents subclassing - if a singleton can be subclassed once, it can be subclassed twice, and if each of those subclasses can create an instance, the pattern is violated.
  • The class is sealed. This is unnecessary, strictly speaking, due to the above point, but may help the JIT to optimise things more.
  • A static variable which holds a reference to the single created instance, if any.
  • A public static means of getting the reference to the single created instance, creating one if necessary.

Basically your implementation of the pattern boils down to his first example:

public sealed class Singleton
{
    private static Singleton instance=null;

    private Singleton()
    {
    }

    public static Singleton Instance
    {
        get
        {
            if (instance==null)
            {
                instance = new Singleton();
            }
            return instance;
        }
    }
} 

And this example is bad code. I stringly suggest you read the article to understand all this and implement the correct way of the singleton pattern.


Other points:

Take your following code:

public Cart UserCart
{
    get
    {
        if (HttpContext.Current.Session["Cart"] != null)
        {
            return HttpContext.Current.Session["Cart"] as Cart;
        }
        return new Cart();
    }
    set
    {
        HttpContext.Current.Session["Cart"] = value;
    }
}

This is wrong, the setter should be used to set the value of a Cart instance, certainly not to save a value in the HttpSessionState object. Also since this has all to do with Cart and not with the SessionFactory, maybe this belongs in the Cart class.


Why mix fieldnaming with and without the underscore and capitalize some and do not so with other? Prefixing them with an underscore is personal choice but fieldnames have camelCase, according to the capitalization conventions of Microsoft.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for the feed back. The Post by Jon Skeet is excellent. I have revised my code to reflect the article. As for naming conventions; I am aware. As its a personal choice, I choose whats best in my experience. \$\endgroup\$ – PhillyNJ Jun 9 '15 at 19: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.