This is the mail archive of the cygwin-apps@cygwin.com mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [SetupXP] The two styles for handling activation refusal


On Tue, 2003-07-22 at 00:02, Max Bowsher wrote:
> OK, this is a general reply to multiple messages.
> 
> I still believe "bool OnActivate()" to be the better option - here's why:
> 
> The "if(canActivate()){OnActivate()}" scheme makes 2 method calls where only
> one is required.

Premature optimisation. Activation occurs what - 8 times in a typical
run?

>  It also opens the possibility for OnActivate to be called
> when activation is not desired. The top of each OnActivate has to grow an
> "if (!canActivate) return;", or trust its caller not to do something
> incorrect.

Sure it has to.... Note that this is:
a) A Template Method. (All event handlers are Template Methods in a
general sense). Template Methods depend on the parent Doing The Right
Thing.
b) The exception case - most pages don't care and will always activate.
c) worthy of a 1-line comment above the virtual event handler :
  /* Only called if canActivate() returns true */

Make the common case easy, the exception harder.

> I do not see "bool OnActivate()" as being confusing, nor as less intuitive
> that firing 2 event handlers consecutively. 

There is only one handler. I'm glad that it wouldn't confuse you though
:}.

> It is true that every instance
> of OnActivate has to start returning a value, but I don't see this as
> incorrect.

I do.

> Quoting Robert:
> > Changing OnActivates meaning would conflate things in the method, and
> > make the code harder to read - "what does false mean - that Activate
> > failed?".
> 
> I think activating, or deciding not to activate, are tightly bound
> activities, which *should* be handled together. And yes, false does mean
> that Activate failed - because the page's internal logic chose to make it
> happen.

No, it doesn't.
- failing indicates a /problem/.
- not being activated indicates the page shouldn't be shown.

mixing the two up is exactly the sort of conflation I was referring to
as occuring when the two aren't separated. It's ALREADY confused you, as
your statement above indicates: the Antivirus refusal is /not/ a
failure.

> Quoting Robert's Pro and Con points:
> Pro1: * Factor out conflated functionality
> Pro2: * Step us toward some form of MVC pattern
> Pro3: * Improve readability
> 
> Con1: * Add additional logic to an extant method.
> Con2: * Conflate GUI logic with processing logic.
> Con3: * Make setup harder to read.
> 
> Addressing point-by-point:
> 
> Pro1, Pro3, Con3 : IMO, OnActivate is the logical and intuitive place to put
> this functionality.

Again, it's not. It's the place a lot of programmers would put it - but
they would rue it in 6 or 12 months. From an OOP point of view, the
evolution might go something like:
bool PageFoo:OnActivate() {
  if (some test)
    return false;
  ...
  return true;
}

"damn, why do I have to hard code this into every derived page. It's
getting tiresome, and we've had juniors adding pages by copying others
and leaving inappropriate tests in place. We also had some guy throw a
fatal exception from the parent when activation 'failed' - (s)he didn't
understand that false isn't a failure. That wasn't that bad though, it
was when the page they had been working on DID fail, but only returned
false and left the entire app unstable that we really had to scramble to
fix things up.

I really need to make it /clear/ that all we are doing is allow the page
to decide itself whether it can be activated..."


> Pro2, Con1: IMO, the extra logic truly belongs here, and any other placement
> contravenes Pro3, Con3.

Sorry, I don't buy this. It doesn't have anything to do with an MVC
pattern. Altering the signature DOES add additional logic to an extand
method, which IS CON1. Saying that the logic /truely/ belongs there
doesn't alter it invoking con1.

> Con2: As a piece of GUI logic, the page class is an appropriate location.
> If, in the distant future, we gain an interactive text UI, then there would
> be reason to factor code out into a new style-neutral UI layer - but that's
> a whole new topic.

We have the skeleton of text processing. In point of fact , I didn't use
con2 for this example - I was making a list of general things I
consider.

> Right, that was my attempt to sell the "bool OnActivate()" style, which I
> believe to be best. However, I do want something to get into setup *soon*,
> so, if this hasn't convinced you, Robert, I will set about extracting the
> minimal set of AcceptActivate() changes from Gary's monster-patch.

It hasn't convinced me. You've asserted your opinion, which I
understand, but you've not swayed me. You've not demonstrated any
benefits I hadn't considered, and you seem to miss the long term costs
that mixing the two functions /will/ incur - even though they have
already affected you!

I repeat the ruling I gave Gary some moons ago:
Use a query method. It's appropriate, less code, easier to adapt in the
future, and a cleaner design.

Rob

-- 
GPG key available at: <http://members.aardvark.net.au/lifeless/keys.txt>.

Attachment: signature.asc
Description: This is a digitally signed message part


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]