Friday, May 1, 2009

Get Rid of Dead Code

How many Siebel developers believe that the best way to remove lines of code from an existing script is to put some comment characters in front of the un-needed code, preventing it from executing. Many developers will copy an existing line of code to change an operation or add a condition, "commenting" the original version instead of deleting it. -- Added 5/3

Good coding practice tells us to diligently and carefully comment and annotate changes to scripts, right? Lets take a step back for a moment and think it through. I'm not certain that the commented lines of script are very helpful, at least in most cases.

First of all, commented script rarely helps to trace the historical changes made in a script. If the commented changes are dated, as they often are, you might find the date helpful. But script comments don't lend themselves to historical analysis nearly as well as versioning. Comparing a previous version of a script with the current version, in a source control repository for example, is much easier than trying to decipher the versions from comments in the script itself. Especially considering that the comments from several changes are often mixed in together.

Secondly, commented script doesn't help much in understanding how the current version of the script works. Lets face it: when you examine a script, you are not looking at the code that is commented. You skip the comments and focus on the code that is still active. If the developer happened to leave some actual words among all the dead code, you might stop to read it, but you certainly won't spend much analysis to find out how the script used to work.

Thirdly, and perhaps most importantly, dead code can cause significant overhead on the Siebel server. Every development team that habitually comments unused blocks of script will soon find that an entire event handler has been commented. In fact, it is common to find Siebel repositories with multiple event handlers entirely commented out. Also common are scripted event handlers where no script is present at all. Event handlers are marked as scripted even if the script contains no executable code except the application-provided stub:

function WebApplet_PreCanInvokeMethod (MethodName,
&CanInvoke)
{
return (ContinueOperation);
}

These event handlers cause a measurable increase in CPU load, especially when the event is frequently encountered. The empty scripts also become a maintenance issue, increasing the time required to compile an srf.

So, get rid of all that dead code. If a scripted event handler is empty, completely delete the script record in the Tools list applet. If you are removing individual lines of script, make sure you fully document your changes in design documents, and use a source control system to keep previous versions of the script. But keep the script itself clean. A clean repository will be easier to maintain and provide less overhead on your Siebel server.

3 comments:

Nitin Jain said...

Hi Jim,

I appreciate your point of view, but I beg to differ with you on adding comments in script. Comments are of immense significance especially in maintainance projects, and those following the agile methodology.

If we have a defect logged pointing to a functionality, and we can trace that validation for eg. to a particular line of code, rather than just remove/alter that validation, it is much easier to just trace the person who added it in the first place. It could possibly have come as a defect from some other tester owing to which the validation was added.

In agile methodology, you would keep getting in new inputs and requirements which are not always well documented. Rather, it would definitely be better to read the comments above the validation code and understand why that part of code is there at all.

At times I have come across fairly large DEV teams comprising 50+ Siebel developers. Sometimes, I had some requirements corresponding to which I was looking into the system for some sample solutions. I ran across scripts written by other team members, and promptly sought their help. I could find who did it as it was there in the comments.

Nitin Jain said...

yes, I definitely agree that at times it goes beyond control and you would see so many scripts being commented and just left in the system, these even proceeding to Production - definitely not acceptable.

People leaving code on the EventHandlers even though rest of scripts have been deleted - not acceptable - performance. But, I guess people do this more because of ignorance than intentions.

Jim Tanner-Uicker said...

Hi Nitin,

Thank you for your comments.

On re-reading my post I can see that it is easy to get the impression that I'm talking about actual comments, rather than lines of eScript that have been "commented" by adding the comment characters to the front of the line. I was, in fact, posting about the latter. I added a paragraph to the front of the post to hopefully clarify.

Obviously, descriptive comments, including the author's name, the date, and an explanation of the change or reference to external documentation are very important, and should be added both in-line and in a comment header.

Thanks for your input.
-- Jim