Is embedding CSS/jQuery code in C# code bad?

Question

I see this type of code when looking through our working code base:

Click to copy

    private Button AddClearButton()
    {
        return new Button
                   {
                       OnClientClick =
                           string.Format(@"$('.{0}').css('background-color', '#FBFBFB');
                                   $('#' + {1}).val('');
                                   $('#' + {2}).val('');
                                   return false;", _className, _hiddenImageNameClientId, _hiddenPathClientId),
                       Text = LanguageManager.Instance.Translate("/button/clear")
                   };
    } 

or

Click to copy

            _nameAndImageDiv = new HtmlGenericControl("div");
            var imageDiv = new HtmlGenericControl("div");
            imageDiv.Attributes.Add("style", "width: 70px; height: 50px; text-align: center; padding-top: 5px; ");

            var nameDiv = new HtmlGenericControl("div");
            nameDiv.Attributes.Add("style", "width: 70px; word-wrap: break-word; text-align: center;");
            var image = new HostingThumbnailImage();

Disclaimer: I have not worked with CSS before. but I heard that we should separate css, js, html, C#, other than put them together.

so, is the above code bad? If yes, how is the better approach?

Answer 1

Off the top of my head I can think of a couple of issues, non fatal however.

In no particular order:

1. You lose the ability to cache your JavaScript files, on either the server or on the client.
2. You increase the side of your page. If every button has a lot of embedded JavaScript, then the page size, thus load times, are increased.
3. Debugging will become extremely difficult.
4. Unobtrusive JavaScript is your friend!
5. Maintenance becomes more complex as you need to remember where in the C# code that the hard-coded JavaScript strings are.
6. Intellisense is lost. Visual studio has a fantastic JavaScript editor, you lose all of that functionality by hard-coding the strings
7. Did I mention Unobtrusive JavaScript is your friend!
8. You lose the benefits of separation of functionality.
9. If you have duplicate buttons with the same functionality, then you have duplicate code.

I'm sure there is a bunch I have missed.

Answer 2

That isn't CSS but JavaScript, using the jQuery library. You are right to be suspicious, there are several "smelly" parts with this code:

• Use of the OnClientClick results in an onclick="" attribute, which is less nice than binding the event. This is done dynamically, suggesting it happens multiple types.

• Use and hard-coding of background-color - a CSS class would be much better, this color is probably duplicated many times in the code or the CSS files, and require a lot of work to be changed (redeploying the site code, rather then relying on resource files). A better approach is to use CssClass:

  Click to copy

  imageDiv.CssClass = "imageDiv";

  and having in your CSS file:

  Click to copy

  .imageDiv { width: 70px; height: 50px; text-align: center; padding-top: 5px; }

  this allows you to easily change the design, and having different imageDiv styled best on its context (for example, it can be smaller when it's in the sidebar, using the selector .sidebar .imageDiv)

• Use of String.Format with JavaScript/CSS isn't pretty. For example, this is valid in JavaScript (and jQuery supported): .css({'color': '#FBFBFB', 'border-color':"green"}). With this code, it should be written as .css({{'color': '#FBFBFB', 'border-color':""green""}}) - escaping double quotes for the string, and curly braces for String.Format.

• As you've mentioned, no separation of data/presentation/behavior.

Answer 3

The generated code is Javascript actually, although it manipulates the CSS of some elements.

I'd say the best way would be to execute it at the loading of the page. If all you need is to bind a function to click event, you can do it all in Javascript/JQuery, with something like this:

Click to copy

$("#<%= this.TheButton.ClientID %>").click(function () {
    $("...").css("...", "...");
    // ...
});

I suspect ASP.NET currently simply generates a button with onclick=..., which is generally considered as a bad practice for Javascript programming, but it's not a huge problem.

The general problem here, in my opinion, is that the view and model logic are probably mixed together, but it's difficult to avoid it in traditional ASP.NET.

Answer 4

The answer to the point:

Is the above code bad?

Bad, in the sense "bad programming practice", YES. Definitely bad.

What is the better approach?

The better approach is to split your code into

• Event generating component (You dont have to worry about this)

• Event listener(Which you will have to code)

Why is it a better approach?

Because its good programming practice and this brings in a ton of advantages. This is a subject by itself, which all graduates have to study these days. :D