The New Job, Day 1

[woodwardiocom]

-My first day on the job was hampered by the fact that A) I'm seriously night-shifted, and B) I had to get up at 5:30 to drive the Momster to her bus. So I had a few nanonaps at the keyboard. Still, everyone I've met so far seems nice, and the job is theoretically doable. (It remains to be seen if I'll get the equipment I need -- I obviously can't work on the live server, the backup server is dead, and you have to jump through bureaucratic hoops to set up a new server.)

-I've also discovered that 17 months unemployed didn't make me miss cubeville, uh-uh, nosiree. Suck my will to live away, o flourescent lights of shadowed ennui! But, still very glad to have a job, no matter how short-term. Every three days I work is currently about a month's expenses, assuming I pay off no debt.

-I've also repeated a phenomena I've noticed before, which is that whenever I look at someone else's code, I think, "Woah, that's badly written." Which either means I have an idiosyncratic, eccentric style, or I'm a damn good programmer. For example, in the code I looked at today (with some modifiers to protect corporate IP):


Bad code:
(Note that the indentation is getting scrod here, but I'm too beat to fix it.)
function setLitSessionVars(RitType, Checked)
If RitType = "WW" Then
If Checked = "Y" then
Session.Contents("WW") = "WW"
else
Session.Contents("WW") = ""
end if
End If
If RitType = "AS" Then
If Checked = "Y" then
Session.Contents("AS") = "AS"
else
Session.Contents("AS") = ""
end if
End If
If RitType = "LP" Then
If Checked = "Y" then
Session.Contents("LP") = "LP"
else
Session.Contents("LP") = ""
end if
End If
If RitType = "SC" Then
If Checked = "Y" then
Session.Contents("SC") = "SC"
else
Session.Contents("SC") = ""
end if
End If
If RitType = "Q2" Then
If Checked = "Y" then
Session.Contents("Q2") = "Q2"
else
Session.Contents("Q2") = ""
end if
End If
If RitType = "3P" Then
If Checked = "Y" then
Session.Contents("3P") = "3P"
else
Session.Contents("3P") = ""
end if
End If
If RitType = "TR" Then
If Checked = "Y" then
Session.Contents("TR") = "TR"
else
Session.Contents("TR") = ""
end if
End If
end function


-You repeat something seven times, you have seven chances to get it wrong. My changes:

Better code:
Function setRitSessionVars( RitType, Checked )
If ( RitType = "WW" ) Or _
( RitType = "AS" ) Or _
( RitType = "LP" ) Or _
( RitType = "SC" ) Or _
( RitType = "Q2" ) Or _
( RitType = "3P" ) Or _
( RitType = "TR" ) Then

If Checked = "Y" Then
Session.Contents( RitType ) = RitType
Else
Session.Contents( RitType ) = ""
End If

End If
End Function


Even better code:
Make it a sub, not a function, since it doesn't return anything.
Use some pseudo-Hungarian notation (like strRitType and strChecked) to prevent stooopidity.
Since those seven two-letter constants might change, load them dynamically or set them in an initialization function.
Don't use session variables in the first place, since they chew up memory something fierce.
See if it's possible to pass in a boolean instead of "Y".

Maybe too fancy code:
Replace the first If with:
If InStr( "|WW|AS|LP|SC|Q2|3P|TR|", RitType ) > 0 Then
...And, if this was C, I'd break out the ?: ternary operator instead of the second If. But then, I think ?: is the bee's knees, and I hardly ever got to use it back when I was writing C, mope, pout.

Comments

[signsoflife.livejournal.com]

Dear god. I *just* used the phrase "bees knees" today.

I'm doomed.

ps I am so happy to see you working. (*cranky* one of us should be)

[woodwardiocom.livejournal.com]

I'm doomed.

-I have a window into your skull, Kate. Fear me.

I am so happy to see you working.

-Merci.

[signsoflife.livejournal.com]

Shouldn't you be in bed, Mr. Night-shifted took-Momster-to-the-airport nanonap Man?

[woodwardiocom.livejournal.com]

-I have to get up around 7:30, so I'm going to go to bed at 11. Going to bed early won't fix things.

[ceo]

All good ideas. This is Visual Basic, right? I'd keep all the operators the same case ("end if" or "End If"). Of course, I'm having cognitive dissonance at the idea that they aren't case sensitive.

For your too-fancy code, think about which one involves more string comparisons. Then again, for seven two-letter strings, that's probably premature optimization, unless this is getting called a whole lot.

Hungarian notation is evil. Ideally, the type of your variables ought to be contextually obvious. But again, I'm used to C/C++/Java, where you can't use = for string comparisons.

There are some atrocious programmers out there. I've had that reaction to code like that, and then I've looked at some other code and thought "I can only dream of being that good". I prefer the latter, actually; it gives me more motivation to get there. I read somewhere that the skill level of software engineers varies by an order of magnitude.

[woodwardiocom.livejournal.com]

This is Visual Basic, right?

-VBScript, so, yah, close enough.

Hungarian notation is evil.

-I find pure Hungarian notation confusing. And in web applications (where almost everything is a string at some point in its life) it's not that useful. But I still prefer it.

[hackard.livejournal.com]

I've also repeated a phenomena I've noticed before, which is that whenever I look at someone else's code, I think, "Woah, that's badly written." Which either means I have an idiosyncratic, eccentric style, or I'm a damn good programmer.

Or you're an editor at heart. PH34R T3H R3D P3N!!

[woodwardiocom.livejournal.com]

Or you're an editor at heart.

-Probably some truth to that, yah.

[anaisdjuna.livejournal.com]

Code makes me horny :-O :-) :-P

[woodwardiocom.livejournal.com]

[grin] Next time you're in Boston, I promise to talk nerdy to you.

[anaisdjuna.livejournal.com]

Ooh la la! I'll keep an eye on the Air Tran specials...

I love Boston/MIT- the garden of geekplenty!

;-)

[notthebuddha.livejournal.com]

You repeat something seven times, you have seven chances to get it wrong.

Your predecessor may have worked at one of the places where they gauge productivity by lines of code produced per day. Seriously.

Better code:
Function setRitSessionVars( RitType, Checked )
If ( RitType = "WW" ) Or _
( RitType = "AS" ) Or _
( RitType = "LP" ) Or _
( RitType = "SC" ) Or _
( RitType = "Q2" ) Or _
( RitType = "3P" ) Or _
( RitType = "TR" ) Then

If Checked = "Y" Then
Session.Contents( RitType ) = RitType
Else
Session.Contents( RitType ) = ""
End If

End If
End Function


I'd nest the conditionals the other way around, making the seven string comparisons contingent on the one, so you have a chance to get the job done with only one string lookup and string comparison instead of requiring eight string lookups and eight string comparisons *every* time.

[woodwardiocom.livejournal.com]

-If the RitType isn't one of the legitimate seven strings, I don't want to do anything. Both outcomes of the "If Checked" test depend on RitType being okay. Ergo, I have to look and see what it is, every time.

-Also note that in many programming languages, it won't do eight comparisons every time. Since we have chained Ors, it will fall through as soon as one succeeds. We could even apply some heuristics, and list them in order of decreasing likelihood. (E.g., if Q2 is five times more likely than the rest, list it first.) Of course, last I checked, VBScript wasn't sensible like that...

-And, we're also dealing with at least two different standards of "better code" here. There's ease of maintainability and readability, as distinct from speed. Unless I'm writing something which truly needs to be fast, I tend to favor easy-to-maintain code over speedy code.