We all know that hard-coding is a bad thing (well, maybe not all of us. At one training several years past, I asked the audience "Does anyone think hard-coding is a good idea?" and one person raised his hand. Um, OK).
You know hard-coding: when you say to yourself "This is never going to change." and so you put the "value" directly in your code, over and over again.
I put "value" in quotes, because many developers think simply of hard-coded literal values when they think of hard-coding. But I think there are many more ways that hard-coding can seep into our programs.
So I invite you to help find all the hard-codings in the procedure below.
Here's the rule: you can only identify ONE HARD-CODING in each comment. I will delete a submission with > 1. It'll be more fun that way. Promise!
I will give everyone a couple of days to submit your ideas, then offer my own views on the subject.
You know hard-coding: when you say to yourself "This is never going to change." and so you put the "value" directly in your code, over and over again.
I put "value" in quotes, because many developers think simply of hard-coded literal values when they think of hard-coding. But I think there are many more ways that hard-coding can seep into our programs.
So I invite you to help find all the hard-codings in the procedure below.
Here's the rule: you can only identify ONE HARD-CODING in each comment. I will delete a submission with > 1. It'll be more fun that way. Promise!
I will give everyone a couple of days to submit your ideas, then offer my own views on the subject.
I'll start with the scale of the l_salary variable. It ought to be driven from the underlying column datatype to which it refers.
ReplyDeleteor maybe you just want to have more strict constraint on precision/scale in this method - who knows? you can't just tell by the code itself ...
DeleteLine 31: -20918 is hardcoded
ReplyDeleteLine 31: 'Something went wrong!' is hardcoded
ReplyDeleteHARD-CODING:
ReplyDeleteLine 01) PROCEDURE process_employee (department_id IN NUMBER)
BEST PRACTICE:
Line 01) PROCEDURE process_employee (department_id IN department.department_id%TYPE)
Line 24: 10000000 is hardcoded
ReplyDeleteLine 19: 10000000 is hardcoded
ReplyDeleteLine 5: 100 (length of variable) is hardcoded
ReplyDeleteLine 4: 9, 2 (scale, precision) of variable is hardcoded
ReplyDeleteLine 10: Hardcoded formula for full name
ReplyDeleteIt's not hard-coding, but I wonder why the cursor is never closed -- even in the exception. That said, using a CURSOR FOR LOOP would eliminate the hard coding implicit in the three variable declarations *and* eliminate problems caused by not closing the cursor explicitly.
ReplyDeletePL/SQL cursors will be closed automatically when they go out of scope, so it's actually unnecessary. Since the cursor wasn't e.g. a package global variable, it'll be closed without any intervention. (A comment to that effect is usually a good idea though.)
DeleteThis doesn't apply to DBMS_SQL however: Those will leak if you don't clean up after yourself explicitly. (Well, but the newfangled conversion of DBMS_SQL cursors to SYS_REFCURSOR adds a wrinkle that I haven't explored.)
Steven F.: Sounds like a nice blog topic ;D
Line 10-12 Database structure is hardcoded
ReplyDeleteAh, should have made the rule more clear: only specify hard-coding on a single line or statement. Still, Anonymous did consolidate hard codings on three lines into a single identification of a type of hard-coding, so I publish it! :-)
ReplyDeleteSurely lines 8-12 constitute one statement formatted over multiple lines ending with the semicolon serving as the statement terminator; thus satisfying the rule.
Delete10000000
ReplyDelete10000000 is hardcoded, there should be one parameter in metadata table say meta_variable having value & name as columns , that should be called and 10000000 should be replaced with that
ReplyDeleteI am agree with this!
DeleteThis comment has been removed by a blog administrator.
DeleteLine 3: NUMBER data type for l_id variable is hard-coding. Should be employees.employee_id%type
ReplyDeleteNot a hard-coding, but a third parameter TRUE on the RAISE_APPLICATION_ERROR wouldn't go amiss, else you're burying the actual error that you're supposed to be griping about with the ORA-20918.
ReplyDeleteLine 19-22 we can imagine a boolean-value function instead of IF
ReplyDeleteIt's not a hard-coding, but entire exception block actually convolutes the actual error message as it will not treat the error, nor will it return to the calling environment the actual message.
ReplyDeleteUsing an old-school explicit FETCH instead of a implicit LOOP is pretty 'ard. It leads to bugs too, as demonstrated - WHEN NOTFOUND should immediately follow the FETCH.
ReplyDeleteNot hard-coding, but LAST_NAME + FIRST_NAME isn't required.
ReplyDeleteNice catch. To remove the "hard coding", put it in a (packaged) function.
Deleteencapsulate the cursor select in a separate procedure
DeleteLine 17: INTO variables - hardcodes the column list. Instead a record variable of cursor%rowtype should be used.
ReplyDeleteSven W.
"WHERE department_id = department_id" seems like a problem. "WHERE department_id = process_employee.department_id", surely?
ReplyDeleteInstead of declaring data types using %type, and then use bulk collect in fetch loop.
ReplyDeleteThe cursor : it should be parameterised
ReplyDeleteline 3-5: scope indicator of variables (l_) is a form of hard coding
ReplyDeleteline 7: the comment has a hardcoded reference to a requirements document
ReplyDeleteline 21: name must_be_ceo is a hardcoding of a functional requirement. if next week plsql developers finally get to earn more than 10000000 too, the name of the procedure must be changed.
ReplyDeleteline 24: parameterlist and order is hardcoded, should be named parameters
ReplyDeleteline 26: by placing the exit here (after processing of the row) you hardcoded that there will always be something to process after a fetch.
ReplyDeleteline 31: you hardcoded that every exception is ORA -20918, thereby loosing the information that you really want/need
ReplyDeleteLine - 19 is hardcoded
ReplyDeleteLine#: 19 and 24 salary scale (10000000) is hardcoded
ReplyDeleteline 24 is hardcoded.
ReplyDelete100000000
ReplyDelete100000000
ReplyDeleteerror code -20918
ReplyDeletethe comma ',' in the select statement.
ReplyDelete'Something went wrong'
ReplyDeleteHi Steven ,
ReplyDeleteI fased the issue . We have to change the sum system privileages to over come for hard code.
example : shared_pool_size parm
here we have to chage the parameter eighter TRUE,FALSE, OTHER NUMBERICAL VALUES.
it will works fine.If we set this as per our requiremnet.
we can change as hard parsin or parsing by changing this setting.
it is knowned issue for me. i was fight a lot about this.but forget the sytanx things.
Regards,
Bhaskar.
Is this plain English? I do not understand this - it may be my clumsiness since English is not my native language. But may I have a translation, please?
DeleteCristi, no, I would not call this plain English. I am glad that Bhaskar took the time to submit a comment, but it is a bit difficult to parse his full intent. I believe it has to do with cursor sharing, but maybe not. Bhaskar, would you care to clarify?
Deleteline 4 hardcoded variable precision
ReplyDeleteline 5 hardcoded variable precision
ReplyDeleteline 19 hardcoded condition value.
ReplyDeleteline 24 harcoded function parameter
ReplyDeleteline 31 hardcoded exception message
ReplyDeleteThe cursor : in an ideal code, this cursor should'nt exist. the data should be fetched from the table via a function.
ReplyDeleteQuestion: if we move a hardcoding (like the IF - as it was mentioned before) into a function, procedure or package wouldn't it be still a hardcoding?
ReplyDeleteI think it is hard (even almost impossible) to decide when we have to split our code into smaller pieces and when to stop doing it since simply assembling these pieces would incur a too high cost.
One benefit of a hardcoding is that it might speed things up. But kindly please understand that I do NOT speak in favor of the hardcoding style of writing the code.
To post a hardcoding which I consider myself from time to time to use, even the existence of the cursor itself is a hardcoding for the following possible reasons (I do NOT state that the reasons are true in this particular situation):
1. They might require many context change between PL/SQL and SQL engines; depending what code is "inside" the cursor loop,
2. They might fool the optimizer that one row is accessed at a time, switching from full table scan to index seek, when a full table scan might have been more efficient; this, again, depends on what we do inside the loop;
I have read the posts above, but I have added some necessary explanations to it so I have put it and I add my consideration to Jay Jay who stated the problem before me.
ReplyDeleteMany thanks to everyone who submitted comments on this post, both to identify specific hard-codings and introduce interesting thoughts as to the subtleties of resolving them (or even leaving them in place selectively!). I believe that for most of the comments, no further comment is needed; the hard-codings are obvious.
ReplyDeleteHere are some reflections on what I would call the less obvious aspects of this code:
1. Why no close of cursor? Good point. First, there is no need to explicitly close the cursor because the PL/SQL engine will manage that for us (as it is a locally declared cursor). Having said that, since you want your code to answer and not RAISE questions, I suggest including one - or getting rid of the explicit loop altogether. Replace with cursor FOR loop, BULK COLLECT, etc. Thanks, Jason, for similar comment and blog encouragement!
2. Individual, constrained declarations (and linked to that the hard coding on line 17 - that the cursor always returns 3 items - nice catch, svenweller - that is often overlooked): much better to declare a record against the cursor %ROWTYPE (or do so implcitly with cursor FOR loop). If you must declare using a constrained type, then consider doing so with a SUBTYPE - check out this video: https://youtu.be/jZW-uLb52xk
3. David Aldridge: yes, good catch on that awful WHERE clause - that’s what happens when you don’t have good naming conventions for parameters and/or do not qualify references to variables/parameters inside SQL statements with their PL/SQL scope name, as in
WHERE department_id = process_employee.department_id
4. Naming convention (l_) is a hard-coding of the scope (from Erik van Roon): Hmmm. That one gives me pause for sure. My naming conventions are so deeply engrained I would never have thought this. So the idea is that if someday I decide to move the variable from local to global scope I would have to change the name. Again: hmmmm. What do others think of this?
I am thinking this is a benefit not a problem. It makes me think twice about making such a move, which is almost always the wrong one (going local to global, anyway).
5. Line 26 containing the EXIT WHEN: great catch! A bug, not so much hard-coding, but something that can esily escape attention.
6. RE: Cristi’s comments on “move IF to function…still a hardcoding?” Well, at some point you reach “bedrock” - the “hard” aspects of your code (values, rules, relationships) must be expressed in code. The way I approach it is: is this location the ONLY place it appears? Can it be universally available to developers from this single point of definition? And can that be done efficiently? If the answer is yes, then that’s as good a place as any to keep that logic.
Happy holidays, everyone!
Steven, fully agree.
ReplyDeleteI definitely think it is a very good thing to show the scope in the identifier name, and always do so myself.
But although it may be a form of hardcoding I don't want to do without it's still a form of hardcoding.
And since the assignment was to find all hardcoding, not to identify stuff I would do differently, it's in the list.
:)
I'm surprised you don't call the SQL itself hardcoding. I thought that was part of your best practice to move it to its own function. Sure, there is not much else here, but there is some and this table ought to be accessed from other places. Or have you changed you mind on weather SQL is hardcoding?
ReplyDeleteAlso, the construction of the name (lname) is an unneeded hardcoding as it is never used.
I posted a brief note on naming conventions for PL/SQL source code here: https://blogs.oracle.com/plsql-and-ebr/resource/Response_To_15_Dec_2015_Plsql_Brain_Teaser.pdf
ReplyDeleteFor the people who got interested after Bryn's response, I would suggest reading the following blog post:
ReplyDeletehttps://oracle-base.com/blog/2015/11/25/plsql-formatting-more-pearls-of-wisdom-from-bryn/
Proved to be a real eye opener for me and gave me something to think about hard ;-)
Without reading any comments or answers, the "hard-coding" I see is:
ReplyDelete1. - The datatype specification for the variable l_salary with a hard-code precision and scale.
2. - The concatentation of last_name and first_name column with a string of a comman in quotes. That could've been made a variable.
3. - The boolean expression in the IF statement comparing the salary to the number 10000000
4. - The user-defined ORA-error number used in the call to raise_application_error
5. - The user-defined ORA-message used in the call to raise_application_error
Sorry for Breaking the rule of "One Hard Coding Per Comment".
ReplyDeleteBoss, Every line has hard coding.
Except these lines.
2 , 9 , 13 , 16 , 20 , 22 , 27 , 28 , 29 , 30 , 32
Line 31
ReplyDeleteThis comment has been removed by the author.
ReplyDeleteLine no 19&24
ReplyDeleteIn Declaration two hard coding is there.
ReplyDelete1. L_salary number(9,2) -- we can declare as number (with type)
2. L_name varchar2(100) – 100 is hard coding
3. Name concatenation – ‘,’
4. L_salary > 100000 -- we can include in input (10000)
5. Raise application_error - error code and description is hard coded