Cookie wrapper in MVC4

Question

I'd like to create a cookie wrapper in my application to help support a DRY approach to coding common cookie methods, provide IntelliSense for cookie-type properties (keys), and protect against typo-errors when working with cookies from multiple classes and namespaces.

The best structure for this (that I can think of) is to apply a base class like such:

abstract class CookieBase{
 protected HttpRequestBase request;
 protected HttpCookie cookie;
 protected string name;
 public HttpCookie Cookie { }
 public string Name { get; }
 public HttpCookie Get(){ ... }
 protected string GetValue(string key){ ... }
 public bool IsSet(){ ... }
 protected void SetValue(string key, string value){ ... }
}

Then for each cookie-type that inherits the base class define properties for each key that should be stored in the HttpCookie, and define a constructor that accepts the Request object as a parameter and instantiates the Cookie property of the base class.

Any ideas on how I might improve this design, or suggestions to alternative approaches to attaining the goals I stated at the start of this post?

• svick: Why do you have Cookie and Get() that seem to do the same thing?
• Josh Wheelock: @svick - that was legacy from a first attempt. I've removed the Get() method.
• dcaswell: httpCookie has a name field. If CookieBase can have two names, one in httpCookie and one in a public property you should document their differences. Also I can't tell how the name is going to get updated.

Answer 1

abstract class CookieBase{

Might be just a coding style nitpick, but in C# the scope-opening brace goes on the next line, like this:

abstract class CookieBase
{

Also (and this is possibly just a personal preference), when an access modifier isn't specified, it's public by default. This may or may not be what's intended - the intent is always clearer when the access modifiers are explicit:

public abstract class CookieBase
{

I like that you're using the Base suffix to denote a base/abstract class - seeing FooBase immediately tells me that the class cannot be instantiated directly.

 protected HttpRequestBase request;
 protected HttpCookie cookie;
 protected string name;

protected members should follow the same casing convention as public members - PascalCase:

protected HttpRequestBase Request;
protected HttpCookie Cookie;
protected string Name;

Now, exposing fields is never a recommended approach. You should expose properties, not fields. Hence:

protected HttpRequestBase Request { get; set; }
protected HttpCookie Cookie { get; set; }
protected string Name { get; set; }

Now this causes a naming clash with some other public members:

 public HttpCookie Cookie { }
 public string Name { get; }

The solution would be to make the properties public, with a protected setter (and remove the clashing public members):

public HttpRequestBase Request { get; protected set; }
public HttpCookie Cookie { get; protected set; }
public string Name { get; protected set; }

Having a method called Get is basically a code smell all by itself. I'd get rid of it, its functionality seems to be already implemented in the Cookie property:

 public HttpCookie Get(){ ... }

The rest of your code is not quite reviewable, I'd suggest you first implement it before your post it for peer review:

 protected string GetValue(string key){ ... }
 public bool IsSet(){ ... }
 protected void SetValue(string key, string value){ ... }
}

---

Is this a good approach? Not sure. abstract classes define abstractions, but your abstract class has nothing that actually warrants being abstract - you have no virtual or abstract members, not even a protected constructor, which leads me to believe that your derived classes might look like this:

public class SomeCookie : CookieBase
{
}
public class AnotherCookie : CookieBase
{
}

...if that's the case, then the base class is probably a bad design and you're probably better off just calling it Cookie and making it a normal class.

If your goal was to define an abstraction, @Malachi's answer is probably accurate - you'd be better off defining these members in some ICookie interface (which the "normal" Cookie class described above could very well be implementing).

Answer 2

I am thinking that you are looking for an Interface rather than an Abstract

I am not sure what all you want to use this for, but I am thinking it might be worth looking into, especially if everything inside this class is something that you must have. the only thing with an interface is that it doesn't supply any code, it just provides the template for classes that implement it. to me it just seems that what you want is in interface, but I cannot be certain that is what will work for you.

check out Abstract Class versus Interface From CodeProject.com or even Interfaces and Abstract Classes From the MSDN Website

an Interface will Define what is needed in the class, and give some intellisense when creating the new class when inheriting the interface, and multiple interfaces can be inherited into one class whereas only one abstract class can be inherited to a class.

again, you will have to look at the differences and see what is going to work better for you.

• Josh Wheelock: The abstract class is needed because the GetValue(string),IsSet(), and SetValue(string,string) methods supply routine logic that is not unique the inheriting classes.
• Malachi: I don't know how complex you want this to be, but maybe you could create the abstract that inherits that Interface, (not sure how this works just shooting out an idea), or you could inherit the abstract and the interface, (can't remember if you can do that or not, again just shooting ideas)