10

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

    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

            _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?

Improve this question
6
  • 5
    Seems like bad practice to me, but its not fatal. Commented May 18, 2011 at 4:25
  • 3
    That isn't CSS but JavaScript, using the jQuery library. Commented May 18, 2011 at 4:26
  • @Vimq1987: do you mean Javascript? As Kobi says, that is javascript library code in the OnClient event. Commented May 18, 2011 at 4:28
  • 1
    @Kobi: my mistake, there's some embedded CSS else where. I edited the answer Commented May 18, 2011 at 4:30
  • Good... bad... who are we to judge without knowing first what it is supposed to do... Commented May 18, 2011 at 4:38

11 Answers 11

Reset to default
19
+50

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.

Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

12

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:

    imageDiv.CssClass = "imageDiv";

    and having in your CSS file:

    .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.
Improve this answer

2 Comments

+1 vote for 3rd reason. It's actually my mistake, I used Resharper to convert to String.Format :-s
@Vimvq1987 - Again, none of these are mistakes per se, but some go against best practice. Use of String.Format is sometimes better (here it's border-line, with a few strings), but it introduces a new error you can make - using curly braces incorrectly.
7

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:

$("#<%= 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.

Improve this answer

Comments

7

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

Improve this answer

1 Comment

"Why is it a better approach?" -> "Because it's good programming practice" is a circular argument. The question is interesting because a number of modern systems - web components spring to mind - fly in the face of this traditional wisdom. Separating out code is like taking the traditional approach of putting config in /etc, code in /lib and data in /var and instead putting all in a single directory called /my-app. There are advantages both ways. One really has to look at specifics to see which wins in each context, imo.
5

Most likely the reason this JQuery was implemented in such was a way was due to having to reference server side control Ids (_hiddenImageNameClientId, _hiddenPathClientId), which, prior to .NET 4 was a bit of work. (See Client Ids in .NET 4)

As far as "correctness", I'd consider this improper as I'd much prefer to see a well defined client side layer in javascript defining this click event. Mixing server and client side code "smells" bad to me and breaks SoC IMO.

Improve this answer

1 Comment

What do you mean with SoC? Google has a lot of definitions for this.
5

I don't like embedding CSS in code. A better approach in both cases in my view is to add a class to the element and then have the CSS in a CSS file. In your first example, the background color is changed with javascript, I would add a class ".addClass('selected')" (or toggleClass) with a name that makes sense. In the second example remove CSS and add a class instead .Attributes.Add("class", "xxx").

Your CSS file will contain stuff like this:

.selected {
    background-color: #FBFBFB;
}
...

I don't like manipulating colors/borders and such in c#/javascript because as the project grows, your presentation information ends up all over the place and changing or overriding a color becomes difficult.

Improve this answer

1 Comment

+1 Exactly, CSS lives to be external. 'Oh..update that button width' is no longer a chore, instead of editing all those inline declarations!
3

The first example is actually runs a javascript (jQuery) when the click event is fired. The second is just adding inline styles. I bet this approach was used to get the client id reference (which was difficult pre-.net 4.0) - although, there are other ways to get this using pure javascript.

Some would say its okay, others would say its bad practice and ugly. It depends on your programming style really. And its definitely not fatal.

Pros: - No separate file needed - faster development (arguably)

Cons: - no clear separation of layers - difficult to maintain and debug as the project grows bigger

There's probably more, but that's all I can think of right now.

Personally, I'd stay away from this kind of code. It makes debugging and maintenance a bit more complicated and its not easy to understand for other programmers that touch your code (especially if you have javascript-only and C#-only programmers). But its nothing that a seasoned programmer can't handle, especially on small projects.

Improve this answer

Comments

3

My answer are some questions:

  • What's better inline styles or having them in a css file
  • What's better mixing the javascript with the HTML or having them in a js file

There is plenty to go over to answer those questions and some considerations that vary x scenario.

Short answer: it is a bad practice mixing it up. Favor css classes over inline styles. You can also use jquery's selector to use attach dynamic behaviors to html elements based on css classes or ids those have. Additionally you can have different behaviors based on a containment relationship.

Improve this answer

Comments

2

This code would be very annoying to debug. You asked about a better solution, It appears that everything that is being done here could be done with just javascript .

If you absolutely need C# logic to do this I would wrap all the javascript code into a function and call that function exclusively from your code-behind.

Improve this answer

Comments

1

The first example is actually runs a javascript (jQuery) when the click event is fired. The second is just adding inline styles. I bet this approach was used to get the client id reference (which was difficult pre-.net 4.0) - although, there are other ways to get this using pure javascript.

Some would say its okay, others would say its bad practice and ugly. It depends on your programming style really. And its definitely not fatal.

Pros: - No separate file needed - faster development (arguably)

Cons: - no clear separation of layers - difficult to maintain and debug as the project grows bigger

There's probably more, but that's all I can think of right now.

Personally, I'd stay away from this kind of code. It makes debugging and maintenance a bit more complicated and its not easy to understand for other programmers that touch your code (especially if you have javascript-only and C#-only programmers). But its nothing that a seasoned programmer can't handle, especially on small projects.

Improve this answer

Comments

0

I believe a lot of people have given great reasons why this is a horrible approach. It does work yes, so does a lot of things, that doesn't mean you should keep doing it this way.

I'm gonna plug my own blog here and purpose a solution to stop you from writing this particular type of code because I used to do things like this without having a better solution myself.

In my blog post, I also link to a SO question I made about this a while back. It shows how this can be used to completely avoid this type of dependency. I'll also happily answer any question regrading this approach If you find that it's difficult to understand what I'm getting at.

Improve this answer

Comments

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.