Writing Maintainable Code
-
Comments posted to this topic are about the content posted at http://www.sqlservercentral.com/columnists/pabdulla/writingmaintainablecode.asp
-
Good common sense Pam! Thanks!
I'd like to add another one please.
Make the code modular (if your system will support it).
Extract frequently used functionality to a separate code module, and call that module where-ever you need to perform the function. This will be far easier to test and maintain than hunting down umpteen different attempts at the same function written by different programmers at different times (and even in different languages!). If you can, write the module so that it is dependent on input of only the minimum of key fields and the data to be processed. For everything else it should be self-sufficient. This may mean it has to look up a few things along the way, but you will be able to make far wider use of it than if it is dependant on a whole gaggle of inputs.
Then document the function so all your programming colleagues know about it. Don't keep it to yourself!
David
If it ain't broke, don't fix it...
-
A basic rule of thumb: if you have to comment the code to explain what you did and why, you should seriously consider rewriting the code.
I was with you all the way until you said that... why is it that people are so against simple embedded documentation?
A simple comment for each SELECT, INSERT, UPDATE, DELETE work miracles when you have to modify someone else's code. Something like...
--===== Mark qualifying customers for rewards for the month
... takes no time at all to write and the reader immediately knows whether or not this section of code has anything at all to do with the required mod... in many cases, the might mean the reader can skip the next hundred lines of code without worry...
...and, quick, what does a WHERE SvcTypeID IN (1,2,3,5,1801) mean? If you are not familiar with the code, it may take you hours to find the right person or the right table (if there is one). If you are familiar with the code, it takes scant seconds to do this...
WHERE SvcTypeID IN (1, --Standalone Long Distance
2, --800 Service
3, --800 Pin
5, --Calling Card
1801 --Bundled Services
)
By the way, I'm sometimes surprised when the ladies use "Rule of Thumb"... they don't realize that it's an old term for how thick a stick you could use to legally to beat your wife with. Maybe, if it were documented, they wouldn't use it as much
--Jeff Moden
RBAR is pronounced "ree-bar" and is a "Modenism" for Row-By-Agonizing-Row.
First step towards the paradigm shift of writing Set Based code:
________Stop thinking about what you want to do to a ROW... think, instead, of what you want to do to a COLUMN.Change is inevitable... Change for the better is not.
Helpful Links:
How to post code problems
How to Post Performance Problems
Create a Tally Function (fnTally) -
Jeff,
People are against simple embedded documentation for at least two reasons. First, comments tend to state the obvious. So if you establish a standard that requires comments, you get things like this:
--Update employee's nameUPDATE EmployeeSet FirstName = @FirstName, LastName = @LastNameThe other reason is that comments don't tend to get modified when the code does. So you run the risk of being misled by documentation when looking at the code is what you really need to do.
And keep in mind that these are standards for all development, not just SQL. Languages like COBOL and VB have paragraph or module names that should describe what is happening in that paragraph or module, like RenameExistingFile. Since that piece of code should do nothing more than rename an existing file, what kind of comments do you need? In your example above, I'd name the stored procedure stp_MarkForMonthlyCustomerRewards.
I think it's important to document code that is either nonintuitive or counter-intuitive. It should help, and at least the next person will know you were thinking something when you did it, rather than randomly generating irrelevant code.
And when you say "in many cases, the might mean the reader can skip the next hundred lines of code without worry", if I saw hundreds of lines of code in any module, paragraph, or stored procedure, I'd already be worried.
Mattie
-
Good article Pam!
I'm with Jeff on the issue of comments though. If you *have* to put comments to tell *what* you did, it's either very complex or poorly designed. But that doesn't mean you should leave them out. "Obvious" comments are helpful to junior developers to whom the code will be less obvious. They often do not understand why one approach was taken instead of another and they're often tasked with maintaining older code long before they get to write new code. Comments are a great teaching tool.
Comments explaining *why* you did something are needed in every block except the most trivial. You can't determine why something was done by looking at code. You can guess, and you might even guess correctly, but you can never be certain because the code only shows the implementation or the *how* something was done. Comments tell the full story and tie the code to the business process.
If you extract just the comments from my code you will be able to see what and why my code does what it does. In fact, I usually write the comments as pseudocode before I ever code the statement itself.
And shame on the developer who doesn't update their comments when they update their code. That's sloppy development and is not excusable.
-
if i ever get to debug / performance tune something that is written by someone else that is as simple as
update table1
set field1 = value1
, field2 = value2
then i will be truly amazed.
however, i get complicated selects with ridiculous joins that i have to work out why on earth they've done it (and every time they'll have left the company before i get called in).
so i have to spend ages trying to work out what they are trying to achieve just from what their code does - which may not even be correct.
if they'd only given me a few clues i could have not wasted my time and the company's money guessing!
one of the ones recently i had to do included "where isnull(column1, '') = value1" and value1 could never be an empty length string.
-
Seeing as the majority seems to prefer commented code, I'd be interested to see how you would document code (presumably SQL) that you've written or maintained. Maybe once I see a good example of it, I'll be convinced!
Thanks,
Mattie
-
Hi Mattie,
Here's a quick example I grabbed that is lightly sprinkled with comments but they're enough to guide the reader. It's not a particularly complex function, but I hope you can see how it gives clues about what's going on without requiring the reader to fully understand the code. Please no one kill me, I realize this is an Oracle function.
CREATE OR REPLACE FUNCTION example.idt_f_get_constant_dt (
/*
| Unit Name: idt_f_get_constant_dt
| File Name: idt_functions.sql
| Purpose: Return a global constant, if a non-date value is encountered, return null
| Parameters: p_constant_nm IN VARCHAR2 the constant name requested
| p_site_cd IN VARCHAR2 the site code requested (defaults to GLOBAL)
| return OUT VARCHAR2 the constant value as a date. If a site
| constant is requested but not found,
| the global constant of the same name is
| returned. Null if no value is found or
| the value is not a date.
|
| Who: Date: Ver: Description (References):
| JTL 05/03/2006 1.0.0 Initial Build
| JTL 06/25/2006 1.14.0 Production Release of [system]
|
*/
p_constant_nm IN VARCHAR2,
p_site_cd IN VARCHAR2 := 'GLOBAL'
)
RETURN DATE
IS
v_data_type_cd VARCHAR2(10);
v_value_txt VARCHAR2(100);
v_value_dt DATE;
v_value_nbr NUMBER;
v_count_nbr NUMBER;
v_return_dt VARCHAR2(100);
BEGIN
/* determine the whether the constant exists */
SELECT COUNT(*)
INTO v_count_nbr
FROM example.idt_constant c
WHERE c.constant_nm = p_constant_nm
AND c.site_cd = p_site_cd;
IF v_count_nbr = 1 THEN
/* found constant, determine data type */
SELECT NVL(c.data_type_cd, 'STRING'),
c.value_txt,
c.value_dt,
c.value_nbr
INTO v_data_type_cd,
v_value_txt,
v_value_dt,
v_value_nbr
FROM example.idt_constant c
WHERE c.constant_nm = p_constant_nm
AND c.site_cd = p_site_cd;
/* set return value */
IF v_data_type_cd = 'STRING' THEN
v_return_dt := NULL;
ELSIF v_data_type_cd = 'DATE' THEN
v_return_dt := v_value_dt;
ELSIF v_data_type_cd = 'NUMBER' THEN
v_return_dt := NULL;
ELSE
v_return_dt := NULL;
END IF;
/* did not find the site-specific constant, so check for a global version */
ELSIF p_site_cd <> 'GLOBAL' THEN
v_return_dt :=
example.idt_f_get_constant_dt(p_constant_nm => p_constant_nm, p_site_cd => 'GLOBAL');
/* did not find the global constant */
ELSE
v_return_dt := NULL;
END IF;
/* return the value */
RETURN v_return_dt;
EXCEPTION
WHEN NO_DATA_FOUND THEN
RETURN NULL;
END idt_f_get_constant_dt;
-
To Joe's point: I think Jeff's documentation is an excellent example of my point. When the user decides that
SvcTypeID IN (1,2,3,5,1801)
doesn't mean
WHERE SvcTypeID IN (1, --Standalone Long Distance
2, --800 Service
3, --800 Pin
5, --Calling Card
1801 --Bundled Services
)
but instead means
WHERE SvcTypeID IN (1, --Standalone Long Distance
2, --800 Service
3, --800 Pin
5, --Calling Card
1801 --Bundled Services, not including VOIP
)
your documentation is out of date, and the code hasn't even been touched. I agree completely that stored procedures need a functional description and a modification history.
And JT, thanks for posting the function, which I couldn't begin to follow (as an Oracle function) without the comments. That said, there are several things that go back to Pam's point about naming conventions. The function name example.idt_f_get_constant_dt could be more descriptive, and the count is selected into a value named v_count_nbr, which is pretty generic too. But Pam made another point, which is to set up a standard, and stick to it. So if everyone in your shop agrees that dt is the abbreviation to use for date, and the v_ means something to everybody, then that's what's important.
As long as it's all written down somewhere.
Mattie
-
One other area not touched upon that makes code easier to read is: "When you update code, make sure you do not leave code that is no longer relevent." For example JT used:
IF v_data_type_cd = 'STRING' THEN
v_return_dt := NULL;
ELSIF v_data_type_cd = 'DATE' THEN
v_return_dt := v_value_dt;
ELSIF v_data_type_cd = 'NUMBER' THEN
v_return_dt := NULL;
ELSE
v_return_dt := NULL;
END IF;
instead of
IF v_data_type_cd = 'DATE' THEN
v_return_dt := v_value_dt;
ELSE
v_return_dt := NULL;
END IF;
If I had to guess, the original function (or its original intent) returned information about NUMBER and STRING data types. But I cannot just "guess" when working on code so I need to double check the code to see if I am missing something, which wastes valuable time.
-
Tom,
Good catch! This function is actually one of 3 versions (_dt, _nbr, and _txt) with a parallel designs. I.e. they all have the same if/then statement for consistency sake. But yes, the code could be condensed in this version.
-
Mattie,
Both the function name and the column names use standard abbreviations that are in the standards guide. We've found it cuts down on the possible ways of naming a column. For instance first name is always "first_nm" and never "first_name", "FirstName", or "FName". The function names are a bit tougher to decode, but they're also standardized to be:
idt = application prefix (used for functions, procs, packages, tables, views, etc)
f = function, as opposed to "sp" for procedure or "pkg" for package (oracle group of related functions)
get_constant_dt = verb-noun set to describe what the object does.
The v_ indicates a local variable as opposed to a g_ for global, c_ for constant, or p_ for parameter. In Oracle it prevents accidently re-using a variable or column name and causing very difficult to locate bugs.
-
Agreed. But then, "I'm with Jeff on the issue of comments though. If you *have* to put comments to tell *what* you did, it's either very complex or poorly designed." does not take into account the many cases where we are deling with importing (or mining) data from somebodyelses tables.
We try to use a good naming convention. We can use my table documentor to extract the table names, column names, and data types. The tech writers cut and paste this into the system manual and only have to fill in comments in rare occasions.
Then I get the joy of having to go get data from SAP, Great Plains, or J. D. Edwards. Not so bad. Next I get an assignment to deal with some home grown system. Now comments every where!
ATBCharles Kincaid
-
Agree with the comments about 'why'. Why isn't used to redundantly explain 'what' but why you made certain choices. It does not explain what the code does, but explains how you decided to default something or why you are choosing some construct that might not be obvious. Sometimes it might be "the PM said blah blah is ok, but I don't trust that and am handling the possibility by doing xyz".
-
Obvious, from reading code?
Try
ReadyStatus = ( ReadyStatus = False )
To an experinced VB programmer it might be appearent but two member of our team could not understand what it did until it was re-written as:
ReadyStatus = NOT(ReadyStatus)
ATBCharles Kincaid
Viewing 15 posts - 1 through 15 (of 35 total)
You must be logged in to reply to this topic. Login to reply