SQL Clone
SQLServerCentral is supported by Redgate
 
Log in  ::  Register  ::  Not logged in
 
 
 


Writing Maintainable Code


Writing Maintainable Code

Author
Message
MattieNH
MattieNH
Hall of Fame
Hall of Fame (3.2K reputation)Hall of Fame (3.2K reputation)Hall of Fame (3.2K reputation)Hall of Fame (3.2K reputation)Hall of Fame (3.2K reputation)Hall of Fame (3.2K reputation)Hall of Fame (3.2K reputation)Hall of Fame (3.2K reputation)

Group: General Forum Members
Points: 3169 Visits: 901

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





Tom Erwine
Tom Erwine
SSC-Addicted
SSC-Addicted (412 reputation)SSC-Addicted (412 reputation)SSC-Addicted (412 reputation)SSC-Addicted (412 reputation)SSC-Addicted (412 reputation)SSC-Addicted (412 reputation)SSC-Addicted (412 reputation)SSC-Addicted (412 reputation)

Group: General Forum Members
Points: 412 Visits: 11

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.


JT Lovell
JT Lovell
SSC-Enthusiastic
SSC-Enthusiastic (124 reputation)SSC-Enthusiastic (124 reputation)SSC-Enthusiastic (124 reputation)SSC-Enthusiastic (124 reputation)SSC-Enthusiastic (124 reputation)SSC-Enthusiastic (124 reputation)SSC-Enthusiastic (124 reputation)SSC-Enthusiastic (124 reputation)

Group: General Forum Members
Points: 124 Visits: 3

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.


JT Lovell
JT Lovell
SSC-Enthusiastic
SSC-Enthusiastic (124 reputation)SSC-Enthusiastic (124 reputation)SSC-Enthusiastic (124 reputation)SSC-Enthusiastic (124 reputation)SSC-Enthusiastic (124 reputation)SSC-Enthusiastic (124 reputation)SSC-Enthusiastic (124 reputation)SSC-Enthusiastic (124 reputation)

Group: General Forum Members
Points: 124 Visits: 3

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.


Charles Kincaid
Charles Kincaid
SSCommitted
SSCommitted (1.7K reputation)SSCommitted (1.7K reputation)SSCommitted (1.7K reputation)SSCommitted (1.7K reputation)SSCommitted (1.7K reputation)SSCommitted (1.7K reputation)SSCommitted (1.7K reputation)SSCommitted (1.7K reputation)

Group: General Forum Members
Points: 1663 Visits: 2384

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
bill page-320204
bill page-320204
Forum Newbie
Forum Newbie (8 reputation)Forum Newbie (8 reputation)Forum Newbie (8 reputation)Forum Newbie (8 reputation)Forum Newbie (8 reputation)Forum Newbie (8 reputation)Forum Newbie (8 reputation)Forum Newbie (8 reputation)

Group: General Forum Members
Points: 8 Visits: 47
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".
Charles Kincaid
Charles Kincaid
SSCommitted
SSCommitted (1.7K reputation)SSCommitted (1.7K reputation)SSCommitted (1.7K reputation)SSCommitted (1.7K reputation)SSCommitted (1.7K reputation)SSCommitted (1.7K reputation)SSCommitted (1.7K reputation)SSCommitted (1.7K reputation)

Group: General Forum Members
Points: 1663 Visits: 2384

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
MattieNH
MattieNH
Hall of Fame
Hall of Fame (3.2K reputation)Hall of Fame (3.2K reputation)Hall of Fame (3.2K reputation)Hall of Fame (3.2K reputation)Hall of Fame (3.2K reputation)Hall of Fame (3.2K reputation)Hall of Fame (3.2K reputation)Hall of Fame (3.2K reputation)

Group: General Forum Members
Points: 3169 Visits: 901

ReadyStatus = ( ReadyStatus = False )

So should this be documented, or rewritten?

Mattie





Stephanie J Brown
Stephanie J Brown
SSC Eights!
SSC Eights! (906 reputation)SSC Eights! (906 reputation)SSC Eights! (906 reputation)SSC Eights! (906 reputation)SSC Eights! (906 reputation)SSC Eights! (906 reputation)SSC Eights! (906 reputation)SSC Eights! (906 reputation)

Group: General Forum Members
Points: 906 Visits: 1103

Put me on the "comment your code" side of the argument (the "why" can be critical six months down the line); and adding white space for readability, too!

Since I work in multiple languages (starting with COBOL back in '80), the "don't use abbreviations" in names is one area I have to disagree with.

If we didn't abbreviate, the COBOL names would get totally out of hand, and take up an entire line by themselves. In my current business, we have standard abbreviations that are used, for instance "Elig" means "Eligibility". Fortunately it's a "spoken abbreviation" as well - a business standard! I'll maintain, for the sake of discussion, that as long as an abbreviation is standardized, it's okay to use them in names of variables, stored procedures, functions, or whatever.

In terms of standards, I agree with Pam that they give you a structure, but getting too detailed with them will cause problems. I worked on a standards committee once where we had a "cross every <T> dot every <I>" member, and it wasted a whole lot of time. A basic set of rules that everyone can understand is essential; anything beyond that is superfluous.




Here there be dragons...,

Steph Brown
Tom Erwine
Tom Erwine
SSC-Addicted
SSC-Addicted (412 reputation)SSC-Addicted (412 reputation)SSC-Addicted (412 reputation)SSC-Addicted (412 reputation)SSC-Addicted (412 reputation)SSC-Addicted (412 reputation)SSC-Addicted (412 reputation)SSC-Addicted (412 reputation)

Group: General Forum Members
Points: 412 Visits: 11
If it doe not affect speed or resources, I would vote for rewrite since the typing is approximately the same. If any of these 3 are affected (speed, resource, or much more typing) then keep the same. As far as documenting, this is where the question of "what is obvious" comes into play. If too many comments are made for the sake of junior programmers, then the "important" comments get lost.
Go


Permissions

You can't post new topics.
You can't post topic replies.
You can't post new polls.
You can't post replies to polls.
You can't edit your own topics.
You can't delete your own topics.
You can't edit other topics.
You can't delete other topics.
You can't edit your own posts.
You can't edit other posts.
You can't delete your own posts.
You can't delete other posts.
You can't post events.
You can't edit your own events.
You can't edit other events.
You can't delete your own events.
You can't delete other events.
You can't send private messages.
You can't send emails.
You can read topics.
You can't vote in polls.
You can't upload attachments.
You can download attachments.
You can't post HTML code.
You can't edit HTML code.
You can't post IFCode.
You can't post JavaScript.
You can post emoticons.
You can't post or upload images.

Select a forum

































































































































































SQLServerCentral


Search