HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Cookie wrapper in MVC4

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
wrappercookiemvc4

Problem

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?

Solution

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).

Code Snippets

abstract class CookieBase{
abstract class CookieBase
{
public abstract class CookieBase
{
protected HttpRequestBase request;
   protected HttpCookie cookie;
   protected string name;
protected HttpRequestBase Request;
protected HttpCookie Cookie;
protected string Name;

Context

StackExchange Code Review Q#30251, answer score: 2

Revisions (0)

No revisions yet.