CodeSOD: World Class Contracting

The time and effort needed to complete a project and the amount of time available rarely line up well. Wayne M's company found themselves in a position where what they needed to deliver and what they could deliver by the deadline simply didn't match up.

The requirements were well specified, so they bundled up a bunch of requirements for search-related functionality, and handed them off to a self-described "world class" developer working on contract.

When the developer handed off their C# code, well, there were some problems. Like the code didn't work. It didn't even compile. Even if it did compile, it was just... bad.

public string SearchText { get; private set; }public object[] Results { get; private set; }protected void Page_Load(object sender, EventArgs e){string searchText = UrlDecode(SetSearch());var results = Search(searchText);results = (from r in results where r.isActive select r).ToArray();OrdersRepeater.DataSource = Results;}private string SetSearch(){SearchText = "";if (Request.QueryString.Get("Search") != null && Request.QueryString.Get("Search") != ""){SearchText = Request.QueryString.Get("Search");}return SearchText;}private object[] Search(string searchText){Results = Repository.Search(SearchText, IsActiveEnum.All);Return Results;}

The cause of the compilation failure is that last return line, or should I say, Return. Which this isn't the only case of capitalization causing issues.

The goal of this code is to take the Search parameter from the URL query string, and run it through the database to find results which can then be displayed in an OrdersRepeater- a server-side UI object that generates HTML output.

The key problem and source of confusion is that there's a property SearchText, and there's a parameter in the Search method called searchText. In the Search method, the property is used for the search, not the parameter, which is just ignored.

Of course, there's no reason to have the property in the first place- any of the properties declared in this snippet. They just exist to create confusion, especially as SetSearch not only sets the property after doing some null checks, but also returns the value. It also checks for an empty string, which is clearly not necessary, as it will return an empty string if the parameter is empty.

I know nothing about the implementation of Repository.Search, but I also have a suspicion that the second parameter allows you to send a pass a filter to select for All entries, ActiveOnly, or InactiveOnly, or something similar. In the Search method we search for All entries, but in the Page_Load event, we use LINQ to filter where r.isActive. I can't prove that they're fetching way more records than they mean to, but we all know they are.

Well, they're not, since this code doesn't even compile, but they wanted to.

[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!