[haiku-commits] Re: haiku: hrev45317 - in src: apps/deskbar preferences/time

  • From: Axel Dörfler <axeld@xxxxxxxxxxxxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Mon, 25 Feb 2013 16:45:43 +0100

Am 25/02/2013 16:31, schrieb John Scipione:
On Feb 25, 2013, at 6:55 AM, "Axel Dörfler" <axeld@xxxxxxxxxxxxxxxx> wrote:
Methods that override the behavior of standard methods should generally be
avoided -- other users of this class might depend or assume standard behavior
(now and in the future).
I am the only user of this class. No methods are virtual. I don't expect this
class to be used by anyone else or used more than once. It's a "singleton" 
class.

That's not the point; I should have been more specific, although you seem to get the idea below.

Instead, you should introduce new methods that do exactly what you want, and
then use them in those cases.
I need to override the Hide() and Show() methods to set a variable that I can 
read later.

Exactly, those are uncritical.

I could create an IsHiddenIgnoreWindow() method though that reads the variable 
i set
in Hide() and Show() and leave the IsHidden() method to do the default.

If there wouldn't be a IsHidden(BView*) this would indeed the way to go.
By changing the functionality of IsHidden() to cater your needs, you lower code reusability, and replace that with a few surprises for other developers who might work on or with that code in the future, and who are familiar with Haiku's API.

Additionally, in this particular case: Is there any need to instantiate the time
view if it's not supposed to be visible anyway?
Only so that we can detect that it is hidden in the preferences and unhide it.
The code could be refactored to handle the case of a NULL TimeView object if 
that
is something we really wanted/needed. If you really don't want to use the system
clock uninstantiating it would save memory.

That would certainly make sense, at least, also from a pure design POV that neglects the memory savings.

It's a good argument for making the clock a status tray replicant. You could
then remove it like any other tray icon.

Indeed :-)

Bye,
   Axel.


Other related posts: