Forum OpenACS Development: content_folder.rename
procedure rename ( folder_id in cr_folders.folder_id%TYPE, name in cr_items.name%TYPE default null, label in cr_folders.label%TYPE default null, description in cr_folders.description%TYPE default null ) is begin if name is not null then content_item.rename(folder_id, name); end if; if label is not null and description is not null then update cr_folders set label = label, description = description where folder_id = folder_id; elsif label is not null and description is null then update cr_folders set label = label where folder_id = folder_id; end if; end rename;
- 2 things:
- rename and delete cannot be used anymore in Oracle9+, all PL/SQL names should be converted.
- The update statements do nothing; variables should be prefixed with the function name.
I fixed it this way:
procedure re_name ( folder_id in cr_folders.folder_id%TYPE, name in cr_items.name%TYPE default null, label in cr_folders.label%TYPE default null, description in cr_folders.description%TYPE default null ) is begin if name is not null then content_item.re_name(folder_id, name); end if; if label is not null and description is not null then update cr_folders set label = re_name.label, description = re_name.description where folder_id = re_name.folder_id; elsif label is not null and description is null then update cr_folders set label = re_name.label where folder_id = re_name.folder_id; end if; end re_name;
up with was .del and .ren for these functions.
Thanks for finding it...
I got sure that I have no more rename and delete using:
select * from user_source where text like '% delete (%' or text like '% rename (%'
But how about the other problem? Calling matching variables in a query without suffixing them??
It is hard to be searched
Samer, thanks for pointing this out. An early aD style guide recommended the use of <proc_name>.variable_name.
I think we should just drop this coding style recommendation and move towards to variables with prefixes e g. v_ or m_. I agree that the aD style guide looks nicer and cleaner, but it is error-prone and it is next to impossible to track down these errors except with constant code review. And why not just avoid bugs that are hard to be tracked down - if it is so easily done?
Here's a good example where I think that this naming convention under Oracle is dangerous:
procedure del ( calendar_id in calendars.calendar_id%TYPE ) is begin -- First erase all the item relate to this calendar. delete from calendars where calendar_id = calendar.del.calendar_id; -- Delete all privileges associate with this calendar delete from acs_permissions where object_id = calendar.del.calendar_id; -- Delete all privilges of the cal_items that's associated -- with this calendar delete from acs_permissions where object_id in ( select cal_item_id from cal_items where on_which_calendar = calendar.del.calendar_id ); acs_object.del(calendar_id); end del;
It admittedly is more stylish, but it is error-prone (without throwing errors) and is potentially dangerous because it may delete loads of items.
Let me dig up a coding style proposal from Feuerstein's books once I am back from the Hamburg bug bash.
proc_name.variable_name a coding style? I thought it was namespace qualification? In Oracle sometimes you need it. Obviously it doesn't exist in pg.
Let me know if we should add to that.
In PL/SQL, referencing variables in the proc_name.variable_name style is overly verbose, hard to read, error prone, and basically disgusting in general. I was never aware of any aD "style guide" saying to do it that way, but if there was, that style guide was very misguided. IMNSHO.
If there was a convenient concise namespace notation that meant the
same thing and did not constantly repeat the name of the
function or procedure everywhere (which you may well want to
change...), something like,
this.variable_name, say, then
that would be nice and maybe worth using. But there isn't, and
constantly referring to the
my_big_long_proc_name_foo.object_id or whatever variable
is just plain obnoxious. Using
q_update_date or a similar convention like that is much
better. And the fact that this same convention also work in
PostgreSQL is a nice added bonus.
v_object_idor some similar convention is much better."
Maybe we should just use the PostgreSQL conventions in Oracle.
Guess that sounds like a TIP right?
Maybe there is a way to do such a conversion semi-automatically, have to think about it.
Andrew, I'm guessing you mean p_* for the passed in parameters, and v_* for the local variables. This convention makes it easy to figure out where a variable came from without having to trace through a bunch of code just to make sure. However, once you have done these two simple changes, sometimes you still need the package.proc.var name qualifiers in Oracle. I seem to remember that cursor packages are picky if you don't do everything exactly right.
(At aD, I think the rest of the Muniversal team and I had written reams of custom PL/SQL in a similar style before most of the aD "core team" started using PL/SQL much at all. [grumble grumble])
If you occasionally need to use Oracle's namespace qualification feature on local variables, fine, that's what it's there for. But it would would much nicer to see it only where it's actually needed, not willy nilly all over the place in order to make the variable names as hard to read as possible.
That old style guide Dirk links to above seems awfully mis-guided on this relatively trivial point. Note that the authors seemed to confuse two unrelated things, they say: "Name parameters as simply as possible" and "To achieve this we must fully qualify arguements passed into procedures" - nonsense, the second statement does not follow from the first.
Personally it never bothered me at all to call PL/SQL functions with parameters named "p_foo" rather than "foo", but if you really hate that, that does not mean that your only other option is to use namespace qualification all over the place. Rather obviously, you can simply declare a "p_foo" or "v_foo" variables near the top of your function, and initialize "v_foo" to the passed in value of "foo". Not necessarily ideal, but for a long function, that is going to be much clearer than prefixing the variable with the function name dozens of times throughout the code.
Andrew, I agree, using the function name to qualify a local variable is a useless idea, an idea that is pretty much solved by appending p_ to every passed in parameter, or in pg, aliasing the same way. And every locally declared variable gets appended with v_. The place to be verbose, if at all, in a variable name is in the meat of the name, not in some prefix. Of course what is clear to me might be confusing to someone else. So the debate may continue.
The problem we face (as always) is legacy code, there are lots of queries out there using named parameters ("param_name => value") in calls to PL/SQL and breaking them is something we were loath to do, given that we knew some folks would be migrated aD 4.2 sites to OpenACS 4 etc.
For new stuff though I'd love to see us settle on that convention if others agree. Do we have something like consensus among those posting to this thread? If so, let's TIP it.
It should be possible to "script"-ize at least parts of such a conversion. I will dig up the sed book.
Otherwise I agree: let's get rid of a prefixed variables at least for all new packages.
Yes, I agree. I thought that was the convention. Everytime I have to look at old code I get confused by the var names. There isn't much to do about old code, but new stuff probably would be much easier to understand and maintain by following these two simple conventions.