views:

68

answers:

1

Hi All,

Quick question.

How would you refactor this Asp.net MVC 2 HtmlHelper? Specifically would it make sense to use the TagBuilder class in this scenario?

        public static MvcHtmlString BusinessDisplayContacts(this HtmlHelper helper, string phone, string cellPhone, 
        string fax, string website, string email, bool hideEmail) 
    {
        StringBuilder sb = new StringBuilder();

        sb.AppendLine("<ul>");
        if (!string.IsNullOrEmpty(phone)) { 
            sb.AppendLine("<li class=\"tel\">");
            sb.AppendLine("<span class=\"type\">Work</span>:");
            sb.AppendFormat("<span class=\"value\">{0}</span>",phone);            
            sb.AppendLine("</li>");
        }
        if (!string.IsNullOrEmpty(cellPhone)) { 
            sb.AppendLine("<li class=\"tel\">");
            sb.AppendLine("<span class=\"type\">Cell</span> Phone:");
            sb.AppendFormat("<span class=\"value\">{0}</span>",cellPhone);
            sb.AppendLine("</li>");
        }
        if (!string.IsNullOrEmpty(fax)) { 
            sb.AppendLine("<li class=\"tel\">");
            sb.AppendLine("<span class=\"type\">Fax</span>:");
            sb.AppendFormat("<span class=\"value\">{0}</span>",fax);
            sb.AppendLine("</li>");
        }
        if (!string.IsNullOrEmpty(website)) { 
            sb.AppendFormat("<li><a class=\"url\" href=\"{0}\">{0}</a></li>",website);
        }
        if (!hideEmail && !string.IsNullOrEmpty(email)) {
            sb.AppendFormat("<li><a class=\"email\" href=\"mailto:{0}\">{0}</a></li>",email);
        }
        sb.AppendLine("</ul>");

        if (sb.Length < 10)
        {
            return MvcHtmlString.Create("");
        }
        else {
            return MvcHtmlString.Create(sb.ToString());
        }
    }

Thanks in advance.

UPDATE:
Thank for all of the constructive comments. In the end I've decided to move the above code into a strongly typed partial view as per @queen3's suggestion.

+2  A: 

One thing I see people missing a lot, is using C# verbatim strings for stuff like that... e.g.

sb.AppendLine("<li class=\"tel\">");
sb.AppendLine("<span class=\"type\">Work</span>:");
sb.AppendLine(string.Format("<span class=\"value\">{0}</span>",phone));            
sb.AppendLine("</li>");

can be made into

sb.AppendFormat(@"
<li class=""tel"">
    <span class=""type"">Work</span>: <span class=""value"">{0}</span>
</li>
", phone);

which is way more readable.

Another thing: I would put all those strings + bool inside an object, like ContactInfo or something, changing the signatur of your helper to BusinessDisplayContacts(this HtmlHelper helper, ContactInfo info) - this way you will be able to add/remove/modify phone numbers and conditions without breaking existing code.

mookid8000
Thanks I'll look into creating a new object to pass in. In regards to verbatim strings, it just my personal preference to not make use of that particular language feature. Thanks for the feedback.
Jeremy
Any reason to avoid verbatim strings?
queen3
Voted question down for not mentioning you do not like verbatim screens in your question.
jfar
@jfar thanks. @queen3 I move a lot between Java and C# so it just makes more sense for me personally to use standard escaping.
Jeremy
@jfar "I don't like your solution" is a valid response and there is no need to vote down the question as a result. Second, it is unreasonable to expect a questioner to list every possible solution and their opinion on it before the answer has even appeared. Perhaps they hadn't even considered verbatim strings a possible answer?
Graphain
@Graphain You're misrepresenting the scenario. This questioner asked about refactoring a string heavy template method. Including details about preferred string techniques is certainly important. Its like asking a SQL question without specifying the underlying database.
jfar
@jfar I think the questioner asked about refactoring HTML tags. Yes they are strings but just because your goto tool is verbatim strings doesn't mean the questioner may have even considered them an option. In my opinion tag builder as they proposed is a far better approach as it will help enforce the DOM validity. I think a far more comparable example is "how should i optimise this SQL query", someone suggesting "oh I'd do this and probably put it in a view" and the OP replying they don't like views but the suggestion is still good.
Graphain