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

Using properties efficiently in inheritance

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

Problem

I have a class called Mailer which has some properties like this

public static string Cname
{
   get
   {
      return HttpUtility.HtmlDecode(HttpContext.Current.Server.UrlDecode(HttpContext.Current.Request["cname"]));
   }
   set
   {
      HttpUtility.HtmlDecode(HttpContext.Current.Server.UrlDecode(HttpContext.Current.Request["cname"]));
   }
}


I am inheriting this property from another class called EventRegistrationReplyMail
like this

public class EventRegistrationReplyMail :Mailer


and using Cname property inside this class. I doubt whether this is the correct way of doing things or not. I am just a beginner in OOP concepts, so please spare me if I am doing anything completely stupid.

Solution

Accessors in C#

You should read on accessors in C# - your setter does not do what it should - it should set the value if Cname. If this has no meaning (you can't change the value of Cname - you should drop the set altogether.

Correct use of accessors:

public string Name 
{
   get 
   { 
      return name; 
   }
   set 
   {
      name = value; 
   }
}


For read only properties:

public static string Cname
{
   get
   {
      return HttpUtility.HtmlDecode(HttpContext.Current.Server.UrlDecode(HttpContext.Current.Request["cname"]));
   }
}


Inheritance and static members

You are asking about efficiency in inheriting properties. I don't know what do you mean by "efficiently inheriting", but static members are not really inherited, you simply get some allowance from calling them in the inherited class...


Inheritance in .NET works only on instance base. Static methods are
defined on the type level not on the instance level. That is why
overriding doesn't work with static methods/properties/events...

Use of properties

When using properties, you imply state of the object you are writing the property on (in this case - Mailer class). From looking at your code, it does not look like its the class's state, but rather a calculation made.

A better choice would be to write it as a utility method, which makes it clear that you are calculating something in it:

public static string ExtractCNameFromCurrentRequest() {
    return HttpUtility.HtmlDecode(
       HttpContext.Current.Server.UrlDecode(HttpContext.Current.Request["cname"]));
}


Or you could use an extension method, which will result in a cool usage syntax, and make clear from where you are extracting the CName:

public static class HttpContextExtension {
    public static string GetCName(this HttpContext context) {
        return HttpUtility.HtmlDecode(context.Server.UrlDecode(context.Request["cname"]));
    }
}


And its usage will be:

string cname = HttpContext.Current.GetCName();

Code Snippets

public string Name 
{
   get 
   { 
      return name; 
   }
   set 
   {
      name = value; 
   }
}
public static string Cname
{
   get
   {
      return HttpUtility.HtmlDecode(HttpContext.Current.Server.UrlDecode(HttpContext.Current.Request["cname"]));
   }
}
public static string ExtractCNameFromCurrentRequest() {
    return HttpUtility.HtmlDecode(
       HttpContext.Current.Server.UrlDecode(HttpContext.Current.Request["cname"]));
}
public static class HttpContextExtension {
    public static string GetCName(this HttpContext context) {
        return HttpUtility.HtmlDecode(context.Server.UrlDecode(context.Request["cname"]));
    }
}
string cname = HttpContext.Current.GetCName();

Context

StackExchange Code Review Q#45577, answer score: 8

Revisions (0)

No revisions yet.