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!
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
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:
The 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.
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
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!
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 DATEIS 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)
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.
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 = 'NUMBER' THEN v_return_dt := NULL; ELSE v_return_dt := NULL; END IF;
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.