It's sometimes hard to say given demo code...
You're going to need to look at where Admit_Client() is being used.
If it's always
GetClient (ClientID);
If client_hair_brown and
client_blue_eyes;
Admit_Client ( ClientID ); // external subprocedure
Endif;
Then I'd lean toward replacing the above with
Admit_Client(ClientID);
or
Admit_Client(ClientID:hairColor:eyeColor:hairLength);
Depends on if the GetClient() is being called just for this, or if
it's already being called for other reasons. You can always use the
second form and have hairColor:eyeColor:hairLength be *OMIT:*NOPASS.
So that Admit_Valid_Client() will only call GetClient() if needed
You don't show an ELSE in the original code, so I have to wonder what
happens when the client doesn't meet the requirements?
It may be that something like so would work well:
if Admit_Client(ClientID) <> SUCCESS;
//handle invalid client
endif;
Honestly, the IMHO original source of your problem is probably the
GetClient(). I assume this is basically doing a CHAIN and returning a
DS with the data from the client table. This violates encapsulation
rules, and as you've seen, isn't really any better than doing the
chain directly; since what it leads to is having business rules
duplicated in your code instead of encapsulated in a procedure.
Having "getters" that you rely on to return a value you then use in
business logic someplace else is a sign that you may not be
encapsulating properly. Getters should be limited to retrieving a
value for display or reporting.
This is also evident in your concern about the Admit_Client name being
misleading. It would seem that your Admit_Client is expected to be
simply a WRITE to the admissions table. You could of course name it
Admit_Valid_Client, but such a name implies to me that there could be
an Admit_Invalid_Client which wouldn't seem to make sense.
A procedure should do more than simply emulate an RPG op-code. Given
an Admit_Client() with some sort of return code, expecting validation
to occur is reasonable.
If it were me, I'd probably have procedures like so:
ADMISSIONS_RegisterClient
ADMISSIONS_GetErrorMessage
ADMISSIONS_GetErrorNumber
Then I'd have
ADMISSIONS_IsClientEligible
or
CLIENT_IsEligibleForAdmission
The choice of where to place the eligibility check is a tough one. To
make it, read up on cohesion and coupling.
At first glance, having ADMISSIONS know the rules for admittance would
seem to make sense. But you wouldn't it to have code like so:
If (CLIENT_GetHairColor(ClientID) = HAIR_BROWN); //opps, Getter
returning data for business logic "Client has brown hair"
This would be ok:
if CLIENT_HasBrownHair()
But seems silly...
I still like the idea of having ADMISSIONS know the rules, especially
if for instance the rules involve more than just attributes of the
client. What I'd end up with might be
CLIENT_MeetsPhysicalAttributesForAdmission()
ADMISSION_MeetsEligibilityRequirements()
ADMISSION_MeetsEligibilityRequirements() might look like:
eligible = CLIENT_MeetsPhysicalAttributesForAdmission(ClientID)
and (ADMISSION_IsClientCurrentlyRegistered(ClientID) = FALSE)
return eligible;
This way ADMISSION knows the overall rules "Client must be meet
certain physical requirements" and "Client can't already be
registered". CLIENT knows the specific physical requirements.
HTH,
Charles
On Thu, Sep 9, 2010 at 7:17 AM, David FOXWELL <David.FOXWELL@xxxxxxxxx> wrote:
Adapted from a real-life case :
GetClient (ClientID);
If client_hair_brown and
client_blue_eyes;
Admit_Client ( ClientID ); // external subprocedure
Endif;
// Admit_Client
GetClient (ClientID);
DoStuff();
Modification : Admit_Client must not be performed if client has long hair.
Does the modification go in Admit_Client, eg,
// Admit_Client
GetClient (ClientID);
If client_hair_long;
return;
Else;
DoStuff();
Endif;
Or in all the callers?
If client_hair_brown and
client_blue_eyes and
not client_hair_long;
Admit_Client ( ClientID ); // external subprocedure
Endif;
This would mean duplication of code, but putting the modification in Admit_Client could be misleading to the person reading the caller's code.
Thanks.
--
This is the Midrange Systems Technical Discussion (MIDRANGE-L) mailing list
To post a message email: MIDRANGE-L@xxxxxxxxxxxx
To subscribe, unsubscribe, or change list options,
visit: http://lists.midrange.com/mailman/listinfo/midrange-l
or email: MIDRANGE-L-request@xxxxxxxxxxxx
Before posting, please take a moment to review the archives
at http://archive.midrange.com/midrange-l.
midrange.com
