0
\$\begingroup\$

Below function checks if a grade exists

  ----------------------------------------------------------------------------
  -- CHECK IF THE VALUES WE PASSED AS PARAMETERS EXISTS AS A ONE ROW RECORD IN THE TABLE
  ----------------------------------------------------------------------------
  function record#exists(
    i_math_grade              pls_integer
   ,i_science_grade           pls_integer
   ,i_english_grade           pls_integer
   ,i_music_grade             pls_integer
   ,i_average                 pls_integer   
  )
  return boolean
  is
    l_cnt                     pls_integer;
  begin
    begin
      select count(*)
      into   l_cnt
      from   grade
      where  math_grade    = i_math_grade
        and  science_grade = i_science_grade
        and  english_grade = i_english_grade
        and  music_grade   = i_music_grade
        and  average       = i_average;
    exception
      when no_data_found then
        return false;
    end;

    return l_cnt > 1;

  exception
    when others then
      raise_fa_err('record#exists');
  end record#exists; 
\$\endgroup\$
1
  • 3
    \$\begingroup\$ Welcome to Code Review. Can you please provide some context? Also, what for is this function? Besides that, your title should say the goal of the code, and not what you want us to do. Improving the code is implied in every single question. \$\endgroup\$ Commented Aug 11, 2015 at 10:39

2 Answers 2

2
\$\begingroup\$

This is very much the same idea I'm using to encapsulate "business logic" queries for re-use in PL/SQL code. So if you're looking for a validation of a concept you'll pass my review :) In a comment to the other answer you said this is just a snippet from a package code - that's good you're using packages. For PL/SQL apps in practice all code should be located in a package - a standalone subprograms a rare.

The notes:

  1. select count is always guaranteed to return a single number so the exception handler for no_data_found is a dead code and should be removed.
  2. If you're just checking for existence there's no point to count beyond 1, thus enter rownum = 1. This also guarantees the select returns either 0 or 1. This might or might not help your query performance (check explain plan).
  3. It's a bit questionable if such an utility subprogram should record the exception or not but more information about your error handling implementation would be required to comment further. I usually don't but I'll let the exception propagate to the caller context and then record the full stack trace there. The good thing is that you re-raise (I just assume that from the name of raise_fa_err).

Modified example with the points #1 and #2 applied:

function record#exists(
  i_math_grade    in pls_integer
 ,i_science_grade in pls_integer
 ,i_english_grade in pls_integer
 ,i_music_grade   in pls_integer
 ,i_average       in pls_integer
) return boolean is
  l_exists pls_integer;
begin
  select count(*)
  into   l_exists
  from   grade
  where math_grade    = i_math_grade
    and science_grade = i_science_grade
    and english_grade = i_english_grade
    and music_grade   = i_music_grade
    and average       = i_average
    and rownum        = 1
  ;

  return l_exists = 1;
exception
  when others then
    raise_fa_err('record#exists');
end record#exists;
\$\endgroup\$
2
  • \$\begingroup\$ thanks a lot Anon user! you rock. :) points #1 and #2 are so true. About the encapsulation concept, it's actually the company's concept not mine. haha I wouldn't know about it actually if it's not part of our coding standards. :D \$\endgroup\$
    – sky
    Commented Aug 12, 2015 at 5:32
  • \$\begingroup\$ @sky That's why there is coding standards (for good and for bad). I have here some standards too (and if I don't like them, I'll change them). \$\endgroup\$
    – user272735
    Commented Aug 12, 2015 at 5:40
1
\$\begingroup\$

This whole thing is nothing you should do with a stored procedure. What you have here is a simple (sub)select:

(SELECT (SELECT COUNT(*) FROM grade WHERE [...]) > 1) AS exists

btw. I think you meant to write >= 1 which would work better when you only have one existing record :D


As already visible from above codesnippet I have some slightly different casing preferences for SQL-related code.

I prefer using Shout-case (ALL CAPS) keywords. I find them especially helpful when working with tables and columns that use a mixed casing style like PascalCase, camelCase or snake_case.

This allows cleaner separation of keywords from other things. Your procedure would look as follows for me:

  FUNCTION record#exists(
    i_math_grade              PLS_INTEGER
   ,i_science_grade           PLS_INTEGER
   ,i_english_grade           PLS_INTEGER
   ,i_music_grade             PLS_INTEGER
   ,i_average                 PLS_INTEGER   
  )
  RETURN BOOLEAN
  IS
    l_cnt                     PLS_INTEGER;
  BEGIN
    BEGIN
      SELECT count(*)
      INTO   l_cnt
      FROM   grade
      WHERE  math_grade    = i_math_grade
        AND  science_grade = i_science_grade
        AND  english_grade = i_english_grade
        AND  music_grade   = i_music_grade
        AND  average       = i_average;
    EXCEPTION
      WHEN no_data_found THEN
        RETURN false;
    END;

    RETURN l_cnt > 1;

  EXCEPTION
    WHEN others THEN
      raise_fa_err('record#exists');
  END record#exists;
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.