Re: design question
Chris Uppal wrote:
Tom Forsmo wrote:
> But it isn't really possible to say more without knowing what the
> system does.
I was moved to another project, so I dont have access to the
documentation any more. Additionally I dont have all the details because
the description was at a junior programmer level with only a module
specification. I.e. they gave me a 3 page technical design document
which outlined the two classes to program and a description of the
methods and variables to add to it. Along with this they gave me a
requriements specification which detailed what tests the finished
classes must comply with.
I did not have time to read the system architecture documents or ask any
questions about why they designed it like this. Additionally I know the
architecture group consists of many people coming straight from
university, and I think the design shows that. (its a big and
prestigious contract with a large the consulting firm in charge, and
they need people so any people are good enough...)
bool preferMonday;
bool preferTuesday;
bool preferWednesday;
bool preferThursday;
bool preferFriday;
bool preferMorning;
bool preferAfternoon;
its quite obvious that this is bad design,
Well, it's not coded how I would prefer -- this seems like an ideal place to
use a BitSet or EnumSet -- but it's difficult to call it bad /design/ without
knowing what these objects are supposed to do (or, perhaps, what they /should/
be supposed to do ;-)
Off the top of my head, it strikes me as a bit odd that the class appears to
have no way of recording "dis-preferences" (I can't think of a real word for
that) like "can't make Monday", "mornings are out", and so on. Also the scheme
seems too weak to be able to express plausible real-world preferences like
"either Monday afternoon or Tuesday morning is OK".
as far as I understood it, its supposed to hold information about
possible appointments, which a person requests with a case worker.
Mark has raised the issue of whether an AppointmentItem (whatever that is)
should be holding preferences at all. He may have a point (see my reply to
him), but if there is a genuine need for this, and if "AppointmentItem" isn't
just a (extraordinarily) badly chosen name for a Preference (or
AppointmentPreference, or DateTimePreference); /and/ if this logic has any real
significance within the application (I'd guess it does); then it seems odd not
to have explicit preference objects somewhere in the system. Making them into
full objects would, amongst other things, allow them to be used more generally
(e.g. attached to people, attached to recurring appointments, etc).
I agree, but I dont know why they did it like that. Maybe its because
they thought they could refactor that part later, or maybe its because
the case worker only needs a signal of some preference, and the person
just needs to accept the date of the appointment after that.
Another reason for suspecting that Preferences should exist separately from
Appointments is that it takes at least two set of (real world) preferences to
forge an agreement -- mine and yours. Maybe the system doesn't need to reflect
that reality (e.g. a system which records a patient's preference, but actual
appointments are scheduled unilaterally by the doctor).
I agree there can be many possible preference designs, thats why I
thought that using a preference list would be much better because you
could add new types of preferences or even an entire new preferences
engine if that is what you want. But at the same time that adds a lot of
complexity to a very very small part of this very very large system.
Another thing is, that I am not sure they need that kind of preference
alternatives. Its a goverment office so its not necessarily up to the
person to decide the appointment time, but rather it might be the case
worker that decides and the preference is just an indication. I am not
sure, though, because its a project to move the service to a more
internet friendly era, so allowing the system to match appoinement
preferences, instead of the case worker doing so, would be a good thing.
In any case, just the very idea of using a several individual instance
variables to record alternative preferences is not a very good design,
at least they could have used an enum array. That way, they could use a
for loop to test for preference match instead of a large and ugly switch
statement.
tom