Amusing or annoying code

[Dr Danyo]

Ok I'll bite,

What value is assigned to Min_Negative ?

Rather than checking the size of the collection, isn't it simpler to just call Collection.isEmpty ? At least that way you never have to worry about someone changing the value of Min_Positive.

Dr Danyo.

[BeeJay]

we have got a global constant called "Min_Positive" which is defined as a Real equal to 0.

One could argue that Min_Positive is a somewhat misleading name for the global constant, given that zero isn't a actually a positive number?!?

I'm with Dr D. on this one, using Collection.isEmpty makes the code more readable as well, even if you have to put in a not statement:

Code: Select all

if not collection.isEmpty ... endif;

... assuming of course the only code within the if statement isn't something like:

Code: Select all

if not collection.isEmpty foreach obj in collection ... endforeach; endif;

Cheers,
BeeJay.

[allistar]

[

Code: Select all

if not collection.isEmpty foreach obj in collection ... endforeach; endif;

Eek - two trips to the server to get two locks instead of one (assuming you're not in load state).

Even worse is when you see code like this (the collection is an exclusive member collection):

Code: Select all

if (allThingies <> null) then foreach thingy in allThingies do endforeach; endif;

[BeeJay]

Eek - two trips to the server to get two locks instead of one (assuming you're not in load state).

... and assuming you're not in lock state (yuck!) and that it's not a stable or frozen collection.

Cheers,
BeeJay.

[Rich Cassell]

I have seen on a few occasions methods which return a boolean actually set an entirely separate global boolean...

Something like...

Code: Select all

doValidate() : Boolean; vars begin if something then globalBool := true; else globalBool := false; endif; return true; end;

The calling method then uses this boolean to continue:

Code: Select all

doValidate(); //Call doValidate if globalBool then // Continue endif; end;

[Stokes]

Ok I'll bite,

What value is assigned to Min_Negative ?

Rather than checking the size of the collection, isn't it simpler to just call Collection.isEmpty ? At least that way you never have to worry about someone changing the value of Min_Positive.

Dr Danyo.

There is only Min_Positive in our system. Here is the comment that was made in 1997 why it was implemented:

"Added, constant for checking that various pieces of data are greater than or equal to zero when we are maintaining them.This is to prevent the use of actual numbers in code."

I always use isEmpty unless we are using a collection that stores active and inactive data, then I will use collection.sizeOfActive > 0

we have got a global constant called "Min_Positive" which is defined as a Real equal to 0.

One could argue that Min_Positive is a somewhat misleading name for the global constant, given that zero isn't a actually a positive number?!?

I'm with Dr D. on this one, using Collection.isEmpty makes the code more readable as well, even if you have to put in a not statement:

Code: Select all

if not collection.isEmpty ... endif;

... assuming of course the only code within the if statement isn't something like:

Code: Select all

if not collection.isEmpty foreach obj in collection ... endforeach; endif;

Cheers,
BeeJay.

Good point! It is good to know I have your support on doing a global search and replace on Min_Positive and getting rid of it once and for all.

[murray]

There is only Min_Positive in our system. Here is the comment that was made in 1997 why it was implemented:
"Added, constant for checking that various pieces of data are greater than or equal to zero when we are maintaining them.This is to prevent the use of actual numbers in code."

Heh, sounds like blind adherence to a dogma. If you dont like the hard-coded 0 you can always use the null keyword.
However (and I am being sarcastic here), it did leave you the opportunity of redefining zero if you so wished!

[ghosttie]

I always use isEmpty unless we are using a collection that stores active and inactive data, then I will use collection.sizeOfActive > 0

I hope sizeOfActive doesn't loop through the collection counting them. You might be better off using conditions to create an active and inactive collection.

[Dr Danyo]

Good point! It is good to know I have your support on doing a global search and replace on Min_Positive and getting rid of it once and for all.

Yeah get rid of it, there is a time and a place for replacing literals (magic numbers) with constants, the key to this, is the literal has to have a specific meaning, e.g DRINKING_AGE, TAX_RATE, make good constants for 18 and 12.5.

Min_Positive doesnt have a specific enough meaning, its too vague and actually makes the code less readable IMHO, than just using null or 0.

see http://www.refactoring.com/catalog/repl ... stant.html for some more dogma.

Dr Danyo.

[BeeJay]

I find the following example of an unnecessary use of io on a method parameter somewhat annoying as well - probably down to people not properly understanding the various parameter usage types:

Code: Select all

doSomething( pSomeCollection : MyCollClass io ); vars someObject : MyClass ; begin someObject := ...; pSomeCollection.add( someObject ); end;

ie: Too many people as soon as they get "cannot update constant" go and add io to the parameter definition.

In the case of the above quasi code they should actually have added input not io, as per the following quasi code, since they are not changing the collection to which the pSomeCollection parameter refers.

Code: Select all

doSomething( pSomeCollection : MyCollClass input ); vars someObject : MyClass ; begin someObject := ...; pSomeCollection.add( someObject ); end;

Cheers,
BeeJay.