3
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Perhaps you could show more of the code like where you are using this and other surrounding code modules??? \$\endgroup\$
    – dreza
    Mar 28, 2014 at 7:29
  • 1
    \$\begingroup\$ Don't use the word "efficiency" to refer to things that have nothing to do with efficiency. \$\endgroup\$ Mar 28, 2014 at 15:53

2 Answers 2

8
\$\begingroup\$

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();
\$\endgroup\$
3
\$\begingroup\$

@UriAgassi made very good points, I'm just going to add a few thoughts:

  • In the context of a Mailer class, the name Cname isn't absolutely clear. One of the benefits of encapsulation, is the possibility for abstraction - the code that uses a Mailer object doesn't need to know/care that the HTTP context has a value called "cname". ContactName is probably a much more meaningful name.

I am inheriting this property from another class called EventRegistrationReplyMail

You're not, it's the other way around: the EventRegistrationReplyMail class is inheriting this property from its base class Mailer.


What you have here isn't a property, it's a method in disguise (see this StackOverflow answer for more info); properties should be simple and straightforward, and shouldn't throw exceptions - methods are actions, properties are data.

Like Uri Agassi, what I'd recomment you have here instead, is a method - this way if any exception is thrown, it will be less surprising (POLS) than if it were a property.

Lastly, I'd also recommend separating what's going on in that one-liner, so as to reduce the number of method calls in a single instruction, which increases readability and can make the code self-explanatory with good variable naming:

public string DecodeCNameFromQueryString()
{
    var encodedName = HttpContext.Current.Request["cname"];
    var decodedName = HttpContext.Current.Server.UrlDecode(encodedName);
    return HttpUtility.HtmlDecode(decodedName);
}

Per Difference between UrlEncode and HtmlEncode, I don't think you actually need to make both calls - perhaps you could test whether this returns the same value:

public string DecodeCNameFromQueryString()
{
    var encodedName = HttpContext.Current.Request["cname"];
    return HttpContext.Current.Server.UrlDecode(encodedName);
}

And then I'm guessing your HttpContext.Current.Request would have other query string parameters, so you'd probably be better off with a more generic method that takes a string parameter representing the query string parameter:

public string DecodeQueryStringParameterValue(string parameterName)
{
    var encodedName = HttpContext.Current.Request[parameterName];
    return HttpContext.Current.Server.UrlDecode(encodedName);
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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