Why is this consider bad practice? or is it? (ASP.Net)_问答_开发者

Would this code be considered bad practice:

<div id="sidebar">
  <% =DisplayMeetings(12)  %>
</div>

This is a snippet of code from the default.aspx of a small web app I have worked on. It works perfectly well, runs very fast, but as always, I am aware of the fact that just because it works, doesn't mean it is OK.

Basically, the DisplayMeetings subroutine outputs a bunch of formatted HTML (an unordered list actually), with no formatting, just the requisite html, and then my CSS performs all the necessary formatting.

The data for generating the list comes from an SQL server database (the parameter controls how many rows to return) and I am using stored procedures and datareader for fast access. This keeps my front-end extraordinary 开发者_如何学编程simple and clean, imho, and lets me do all the work in VB or C# in a separate module.

I could of course use a databound repeater (and probably 6 or more other methods) of accomplishing the same thing, but are they any better? Other than loosing the design-time features of VS2010?

The only thing "wrong" with your approach is that it mixes code and display, which you typically want to avoid wherever possible. If you have to have a procedurally generated portion of HTML (because it's just that difficult to do with controls, or whatever), create a control responsible for generating that HTML and embed it in the larger page/control that contains it.

is the "wrong" part, that the subroutine is returning HTML, or is it that I have a code-snippet executed from within the HTML markup?

Both, in a way. And also neither.

There's nothing directly "wrong" with using <%= foo %>; if there were, it wouldn't be part of the framework. What's "wrong" with it is that it sets up a two-way dependency that you don't necessarily want. Your HTML markup is dependent on and has to know about the code-behind. It has to call the method, which in turn is dedicated to the markup and nothing else. If you want to make changes to your output, you may have to change the method, the markup, or both.

What I'm trying to say is that what you're doing makes your code less maintainable, less flexible to modify the next time you have to open the hood.

If it's the only way to solve a problem, then it's the only way, and there's nothing wrong with that. But it's something that should be avoided, in general, if possible.

How would I have done it? That depends on the situation, frankly. If I could have used a standard component with databinding, I'd have done that. That is always preferred. If the HTML was just too complex to use a component, I'd use a Literal control as a placeholder for the markup, and generate the markup in a separate component--likely a User Control.

The point is that the markup knows nothing about the method used to generate the other markup, it just says "something goes here" and relies on its code-behind to handle deciding which markup to put in there.

It's not something I'd do. I'm not a fan of using <%= %> in Webforms. Sadly, it's also common practice.

What I'd probably do instead is a build a user control to represent the markup I wanted and put that on the form. This has the advantages of making the element more re-usable, testable, and gives you more control over where in the page life cycle the code is called.

A databound repeater can make it somewhat simpler to modify your html when you need to change the layout. It is also nice if you have separate people who work on html vs people who work on the server side code.

My usual rule is to use a repeater for any even slightly complex html. And I do not call methods from my aspx/ascx file - I only insert the values of a protected string variable which I have populated in the code. However, these are personal preferences, and I don't see anything really wrong with what you are doing.

Your code (without seeing it) will likely be faster than a repeater, since there is no data binding involved, but it probably wouldn't be a huge thing unless the page is very, very popular.