I’ve got a question and I’m looking for an opinion. If somebody has written some code and they’re looking for a bit of help with something, is it rude to clean it up if it’s hideous to look at? Now I’m not talking about instances where they used a gazillion variables to store some data instead of, at the very least, an array. I’m talking about the look of the actual code. Often during the course of a day, somebody requests that I take a look at their code and offer them some help. I have no problem with doing that. What irks me is how some people do absolutely nothing to make it easy for anybody else to read and follow their code. For example, reading something like this isn’t fun.
function Dostuff(xval,yval,zval) dim d dim zx,zy,zz dim aname, greeting aname = inputbox("What's your name?","Name Prompt") greeting = "Hello, " & aname & " . I'll yammer on here." msgbox greeting zx=0 zy=0 zz=1 d=(xval*zx)+(yval*zy)+(zval*zz) If d= 0 then dostuff = True else doStuff =FALSE End if end function |
Arg, my eyes! I can’t even begin to understand how this is readable to anyone. Sure, VBScript isn’t case sensitive, but consistency is nice, I think. The use of white space – sometimes overuse and sometimes lack thereof – is mind boggling. Where’s the comments? I’d rather read English to quickly learn what the code will do, rather than determining that by reading the code itself. And the variable names. The variables names!!!
Using the style that I’m acustomed to, I’d probably clean it up to look like below.
' Purpose: Determine if the supplied vector ' is perpendicular to the Z axis. ' I: X value of supplied vector ' I: Y value of supplied vector ' I: Y value of supplied vector ' O: True = perpendicular, False = not Function IsPerpToZAxis(iXVal, iYVal, iZVal) Const ZAXIS_X = 0 ' Z-Axis vector X value Const ZAXIS_Y = 0 ' Z-Axis vector Y value Const ZAXIS_Z = 1 ' Z-Axis vector Z value Dim iDotProd ' dot product Dim strName ' user-supplied name Dim strGreeting ' greeting to user ' Prompt the user for their name and display a useless ' greeting. strName = InputBox("What's your name?", "Name Prompt") strGreeting = "Hello, " & strName & _ " . I'll yammer on here." MsgBox (strGreeting) ' Calculate the dot product of the supplied vector and ' the Z axis vector. If the dot product is 0, then ' the supplied vector is perpendicular to the Z axis. ' Return the result. iDotProd = (iXVal * ZAXIS_X) + _ (iYVal * ZAXIS_Y) + _ (iZVal * ZAXIS_Z) If (iDotProd = 0) Then IsPerpToZ = True Else IsPerpToZ = False End If End Function |
Yes, I could’ve created my own point class or used an array for the Z axis, but that’s not the point I’m trying to make. I think the latter code snippet is much more readable than the former, don’t you?
The real question is, though, is it ok to modify somebody’s code to make it more readable and then send it back to them like that. Or should you preserve the way they did things in the first place?
One Response to "wow, you're ugly"
funny you mention this. I am currently storing all my xml data into a bunch of static flash variables when I could easilly be using an array. However, it works just as good and no one will never know but me. I wish I had more time to be a cleaner coder.