1

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>";
7
  • Like on a scale of 1 to 10, 10 being worst? Feb 25, 2009 at 23:44
  • 2
    Sometimes, you just have to do some casting :P Feb 25, 2009 at 23:44
  • @Wayne Koorts: These go to 11.
    – Dan Lew
    Feb 25, 2009 at 23:45
  • How is MultipleTestPasses defined?
    – strager
    Feb 25, 2009 at 23:45
  • 1
    But surely you can mock something for being horrible to read. My eyes hurt. Feb 26, 2009 at 0:06

12 Answers 12

14

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'

1
  • 1
    @kjack, he might be writing a driver.
    – Haoest
    Feb 26, 2009 at 1:06
8

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.

2
  • You get my up vote because you clearly demonstrated that instead of whining about a problem you can simply do something about it that doesn't take a lot of effort. The code is now perfectly readable. Well done!
    – Luke
    Feb 26, 2009 at 2:54
  • 1
    Of course it still has the HTML-injection->XSS vulnerability when subject contains ‘<’...
    – bobince
    Feb 26, 2009 at 12:35
7

Did it pass its unit test?

3
  • @Ainab that's one majore problem right there. Code without a (unit) test should be assumed to be broken. Feb 26, 2009 at 0:39
  • @saua onoe, all my projects are broken, they are making the company alot of money though, so broken is probably overrated.
    – Haoest
    Feb 26, 2009 at 1:08
  • 2
    Sorry, I'll remove my tongue from my cheek now... Code without a unit test may or may not be broken. Code with a unit test may pass the unit test, but is the unit test 'broken', i.e. is it in fact completely testing the desired behavior? Ah, code philosophy. They should run courses on it... Mar 4, 2009 at 23:35
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.

0
2

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)

2

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

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

1

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

1

HTML wrapped up eww.

1

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.

1

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.

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

Oh... my eyes...

1

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

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.