Tuesday, December 15, 2015

PL/SQL Brain Teaser: Find all the hard-codings!

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.


67 comments:

  1. 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.

    ReplyDelete
    Replies
    1. or 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 ...

      Delete
  2. Line 31: 'Something went wrong!' is hardcoded

    ReplyDelete
  3. HARD-CODING:
    Line 01) PROCEDURE process_employee (department_id IN NUMBER)
    BEST PRACTICE:
    Line 01) PROCEDURE process_employee (department_id IN department.department_id%TYPE)

    ReplyDelete
  4. Line 5: 100 (length of variable) is hardcoded

    ReplyDelete
  5. Line 4: 9, 2 (scale, precision) of variable is hardcoded

    ReplyDelete
  6. Line 10: Hardcoded formula for full name

    ReplyDelete
  7. It'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.

    ReplyDelete
    Replies
    1. PL/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.)

      This 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

      Delete
  8. Line 10-12 Database structure is hardcoded

    ReplyDelete
  9. Ah, 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! :-)

    ReplyDelete
    Replies
    1. Surely lines 8-12 constitute one statement formatted over multiple lines ending with the semicolon serving as the statement terminator; thus satisfying the rule.

      Delete
  10. 10000000 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

    ReplyDelete
    Replies
    1. This comment has been removed by a blog administrator.

      Delete
  11. Line 3: NUMBER data type for l_id variable is hard-coding. Should be employees.employee_id%type

    ReplyDelete
  12. Not 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.

    ReplyDelete
  13. Line 19-22 we can imagine a boolean-value function instead of IF

    ReplyDelete
  14. It'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.

    ReplyDelete
  15. Using 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.

    ReplyDelete
  16. Not hard-coding, but LAST_NAME + FIRST_NAME isn't required.

    ReplyDelete
    Replies
    1. Nice catch. To remove the "hard coding", put it in a (packaged) function.

      Delete
    2. encapsulate the cursor select in a separate procedure

      Delete
  17. Line 17: INTO variables - hardcodes the column list. Instead a record variable of cursor%rowtype should be used.

    Sven W.

    ReplyDelete
  18. "WHERE department_id = department_id" seems like a problem. "WHERE department_id = process_employee.department_id", surely?

    ReplyDelete
  19. Instead of declaring data types using %type, and then use bulk collect in fetch loop.

    ReplyDelete
  20. The cursor : it should be parameterised

    ReplyDelete
  21. line 3-5: scope indicator of variables (l_) is a form of hard coding

    ReplyDelete
  22. line 7: the comment has a hardcoded reference to a requirements document

    ReplyDelete
  23. line 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.

    ReplyDelete
  24. line 24: parameterlist and order is hardcoded, should be named parameters

    ReplyDelete
  25. line 26: by placing the exit here (after processing of the row) you hardcoded that there will always be something to process after a fetch.

    ReplyDelete
  26. line 31: you hardcoded that every exception is ORA -20918, thereby loosing the information that you really want/need

    ReplyDelete
  27. Line#: 19 and 24 salary scale (10000000) is hardcoded

    ReplyDelete
  28. the comma ',' in the select statement.

    ReplyDelete
  29. Hi Steven ,

    I 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.

    ReplyDelete
    Replies
    1. 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?

      Delete
    2. Cristi, 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?

      Delete
  30. line 4 hardcoded variable precision

    ReplyDelete
  31. line 5 hardcoded variable precision

    ReplyDelete
  32. line 19 hardcoded condition value.

    ReplyDelete
  33. line 24 harcoded function parameter

    ReplyDelete
  34. line 31 hardcoded exception message

    ReplyDelete
  35. The cursor : in an ideal code, this cursor should'nt exist. the data should be fetched from the table via a function.

    ReplyDelete
  36. Question: 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?

    I 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;

    ReplyDelete
  37. 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.

    ReplyDelete
  38. Many 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.

    Here 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!

    ReplyDelete
  39. Steven, fully agree.
    I 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.
    :)

    ReplyDelete
  40. 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?

    Also, the construction of the name (lname) is an unneeded hardcoding as it is never used.

    ReplyDelete
  41. 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

    ReplyDelete
  42. For the people who got interested after Bryn's response, I would suggest reading the following blog post:

    https://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 ;-)

    ReplyDelete
  43. Without reading any comments or answers, the "hard-coding" I see is:
    1. - 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

    ReplyDelete
  44. Sorry for Breaking the rule of "One Hard Coding Per Comment".
    Boss, Every line has hard coding.
    Except these lines.

    2 , 9 , 13 , 16 , 20 , 22 , 27 , 28 , 29 , 30 , 32

    ReplyDelete
  45. This comment has been removed by the author.

    ReplyDelete
  46. In Declaration two hard coding is there.
    1. 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

    ReplyDelete