7
\$\begingroup\$

I was asked to give more context. So here we go.

We want to create some HTML in code behind to add an "add" and a "refresh" button to a grid view header column. Approach #1 does this using string concatination to produce the necessary HTML, whereas approach #2 uses .NET objects to create exactly the same HTML.

This time I'm posting the complete classes:

Approach #1

class AddBtnTemplate : ITemplate {
    StatRefTimeGrid Parent;
    public AddBtnTemplate(StatRefTimeGrid parent) {
        Parent = parent;
    }
    public void InstantiateIn(Control container) {
        GridViewHeaderTemplateContainer ctr = (GridViewHeaderTemplateContainer)container;
        string html = "<div style='width:150px'>";
        if (Parent.MayEdit)
            html += string.Format("<img alt='{0}' title='{0}' src='Img/add18d.png' style='cursor: pointer' onclick='StatRefTimeGrid.addRow()' />", Texts.CompValue.AddRefTime[Parent.Parent.State.LangIdx]);
        if (ctr.Grid.VisibleRowCount > 1)
            html += string.Format("<img alt='{0}' title='{0}' src='Img/refresh18d.png' style='cursor: pointer' onclick='StatRefTimeGrid.refresh()' />", Texts.CompValue.RefreshRefTimeGrid[Parent.Parent.State.LangIdx]);
        html += "</div>";
        ctr.Controls.Add(new LiteralControl(html));
    }
}

Approach #2

private class AddBtnTemplate : ITemplate {
    private readonly StatRefTimeGrid mParent;
    public AddBtnTemplate(StatRefTimeGrid parent) {
        mParent = parent;
    }
    public void InstantiateIn(Control container) {
        GridViewHeaderTemplateContainer templateContainer = (GridViewHeaderTemplateContainer)container;

        HtmlGenericControl div = new HtmlGenericControl("div") {
            Style = {[HtmlTextWriterStyle.Width] = "150px"},
        };
        if (mParent.MayEdit) {
            AddNewImage(div, Texts.CompValue.AddRefTime[mParent.Parent.State.LangIdx], "~/Img/add18d.png", "StatRefTimeGrid.addRow()");
        }
        if (templateContainer.Grid.VisibleRowCount > 1) {
            AddNewImage(div, Texts.CompValue.RefreshRefTimeGrid[mParent.Parent.State.LangIdx], "~/Img/refresh18d.png", "StatRefTimeGrid.refresh()");
        }

        templateContainer.Controls.Add(div);
    }
    private void AddNewImage(HtmlGenericControl div, string altText, string imageUrl, string onClick) {
        div.Controls.Add(new Image {
            AlternateText = altText,
            ToolTip = altText,
            ImageUrl = imageUrl,
            Attributes = {
                ["onclick"] = onClick,
            },
            Style = {
                [HtmlTextWriterStyle.Cursor] = "pointer",
                [HtmlTextWriterStyle.Position] = "relative",
                [HtmlTextWriterStyle.Top] = "3px",
            },
        });
    }
}

I'm looking for arguments for or against either solution. What do you think?

\$\endgroup\$
3
\$\begingroup\$

Second approach is better, according to the rule: do not repeat the code!

Don't use '+=' to join string, in your case better to use StringBuilder:

var sb = new StringBuilder();
sb.Append();
...
sb.Appden();
...
return sb.ToString();

I think this is better way, StringBuilder is made for this. You can refactor this code better little more :-)

\$\endgroup\$
  • \$\begingroup\$ Thank you for your opinion. Actually I was looking for arguments for or against the server-side controls vs. plain HTML. I didn't mean to give the string solution a disadvantage by repeating code and using += instead of StringBuilder. \$\endgroup\$ – Kim Homann Jul 9 '18 at 10:40
3
\$\begingroup\$

The two main things that a control object gives you is 1) easier access to setting properties and 2) automatic HTML encoding, at the cost of more lines of code (in general). In your case, the HTML is simple enough and there are few enough branches that I'd be ok with the LiteralControl approach.

The one thing to watch out for is that your alt and title strings could easily contain apostrophes (maybe it's not likely in English, but do you localize to French?), and that will break your HTML. I like to keep the string tables clean of formatting, so that means the values have to be encoded before rendering. If you use the control objects, this will be taken care of. If you use the string approach, you have to explicitly call an encoding function. All strings should be run through this function, even if none of the current translations have any problem characters. If there are strings that never need encoding (because they are e.g. widths or positions that depend on the language) put those in a separate string table.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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