How bad is this code?

Question

I have inherited an online quiz application written in C# with these lines of code all over the place.

So how bad is this code?

What are the potential issues I could run into?

How could I improve it?

The code:

strTestPasses += "<tr valign=\"top\"><td><b>Subject</b></td><td>" + ((Hashtable)((ArrayList)((Hashtable)MultipleTestPasses[i])["HasMultipleDataSet"])[j])["subject"] + "</td></tr>";

Answer 1

Well, subjectively, I'd say it's ugly as sin.

Answer 2

It's not great, but I've seen worse.

In the interest of not shocking anyone into a coma or other unpleasant episode, I'll refrain from posting any examples.

Answer 3

Did it pass its unit test?

Answer 4

HTML wrapped up eww.

Answer 5

Again, as a subjective answer to a subjective question, its not pretty. Then again, if this is a smallish website, it may have been easier to do this in the "quick and dirty" way then something more proper, e.g., C# / ASP -> XML -> XSLT -> HTML (or something along those lines anyway - I'm grasping at straws a bit here)

Answer 6

I also hate these 'lines of code'. When people ask me 'Whats the biggest project you have worked on', I say '1 line of code'. When others challenge me thus 'How many lines of code can you write a day?', I say back to them, 'Only one my brother. But it is the one true line'

Answer 7

It's not that bad actually. Obviously that programmer didn't expect somebody will ever take over his project (lack of professional experience) but it shouldn't be hard to refactor those generic hashtables and arraylists into something more meaningful.

Also while refactoring, use string literals with @ for html.

I personally don't care whether he is using any templates for html if it's little project. Templates are often for small projects overkill.

Answer 8

You know how they say "... only a mother could love..."

Well, in this case, only a compiler could love.

Answer 9

To get started refactoring, may I suggest:

TestPassesBuilder.AppendFormat(
    "<tr valign='top'><td><b>Subject</b></td><td>{0}</td></tr>", 
    MultipleTestPasses[i]["HasMultipleDataSet"][j]["subject"]
  );

or

TestPassesBuilder.AppendFormat(
    "<tr valign='top'><td><b>{0}</b></td><td>{1}</td></tr>",
    "Subject", 
    MultipleTestPasses[i]["HasMultipleDataSet"][j]["subject"]
  );

Where TestPassesBuilder is of course a StringBuilder and MultipleTestPasses has been converted to use appropriate generic collection types rather that ArrayList/HashTable abomination. The 2nd option will also allow the title for each row to be factored out into a variable at some point.

For the next step, MultipleTestPasses should be converted to a real object. Since it looks like he's using hard-coded keys, each 'key' really corresponds to a property of a class.

Answer 10

Without knowing anything about size and scope of the code base, I see two things that really bother me about that line of code.

1. HTML layout not separated from content.
2. The Subject should be returned from a descriptively named function.

I'd much prefer to see something like:

strTestPassesStringBuilder.Append(newTableRow("Subject", getSubject());

where the and valign options are controlled via CSS.

Answer 11

((Hashtable)((ArrayList)((Hashtable)MultipleTestPasses[i])["HasMultipleDataSet"])[j])["subject"]

Oh... my eyes...

Answer 12

This is severly bad as it contains the following flaws :

• string concatenation whereas a StringBuilder or a View template could be much better
• View information (such as HTML) in a non-UI layer (probably)
• Ambiguos business logic ("HasMultipleDataSets")
• Untyped datasets (the column index value is string)
• Too much on a single line of code