About Advice 541

SQL Navigator
1

Why 541 advice appears related to an exception definition?

package xpto is

error_size exception;

/*ADVICE(15): Global public variables defined in package specification
[541] */

(…)

end xpto;

I think that that advice should be applied only to variables (public variables)

Thanks

2

Filipe,

The definition on an exception is a definition - just like any other variable.
Anyone having access to the package can use it, if you put it in the package spec.
If that’s not necessary or desired, I’d put the definition at the top of the package body.

3

Yes its a definition but no one is able change it (like a type, etc…) even if its the spec!

It’s not bad codding, its good coding!
Bad codding would be if it was a variable because that would allow the change of the variable from any point.

you agree?

4

Filipe,

It is good coding style - to be able to reuse the same exception in other places i.o. having to redefine them over and over again (possibly with different names).

I’ve generated inline comments for the 541 advice (see below), and this makes it clear why they flag it for all global public variables.
It might be a good idea to make an exception (pun intended) for exceptions, though…

PACKAGE test
IS
error_size exception;
/*ADVICE(3): Global public variables defined in package specification
[541] */
END test;

/* ADVICE:
ADVICE SUMMARY

Count Recommendation

1 [541] Global public variables defined in package specification
              You should always declare package-level data inside the
              package body. You can then define "get and set" programs
              (function and procedure, respectively) in the package
              specification to provide controlled access to that data.
              With this approach, you can guarantee data integrity,
              change your data structure implementation, and also track
              access to those data structures.

*/

5

Soo you agree!

Its not a variable and as Advice 541 ignores types and even constants it should also ignore exception definitions

6

Hi Filipe,

CR has been created for this. For all the code review advisor CR, we wait for Andre to implement them in the new advisor.

Thanks,
Vincent

7

Still happens in build 1317

Filipe

8

Hi Filipe, because the changes need to be done by formatter team but they are working on other tasks, we still have to wait :(. I would say we may have it fixed in the second half of this year when we get the new advisor available.