Code in Methods, not Events. But why????

One of our favorite maxims is “Always code in methods, not events” and I am often asked exactly what we mean by that, and why it matters. After all, VFP provides us with a perfectly workable command button class, that has a Click() event into which we can put code that we want executed when the button is clicked. What’s wrong with that? The short answer is that nothing is wrong with it. But it is not a good idea – and there are several reasons why it isn’t.

First, and most obvious, is that the event’s name describes the situation in which the associated code gets executed (e.g. Click(), GotFocus(), Error() and so on). However, it does not give any indication as to WHAT happens when this event occurs and so there is no easy way when examining code, to determine what is going on. Consider the following code which has to be called when a particular command button has been clicked:

LOCAL lcSceDir, lcDstDir, lcQuoDir, lnCnt, lcToDo, loErr

WITH ThisForm

  *** Get the directory locations

  lcSceDir = .txtSceDir.Value

  lcDstDir = .txtDestDir.Value

  lcQuoDir = .txtQuotDir.Value

  .oMig = NEWOBJECT('xMigrator', 'datamigrator.prg', null, lcSceDir, lcDstDir, lcQuoDir, .T. )

  IF VARTYPE( .oMig ) # "O"

    RETURN

  ELSE

    .UpdProgress( 'Initialization Started at ' + TIME())

    .oMig.oParent = ThisForm

  ENDIF

  FOR lnCnt = 1 TO ALEN( .aProcs, 1 )

    IF .aProcs[ lnCnt, 2 ]

      lcToDo = .aProcs[ lnCnt, 1 ]

      .UpdProgress( 'Start Process [' + lcToDo + '] at ' + TIME())

      .oMig.RunProcess( lcToDo )

      .UpdProgress( 'Ended Process [' + lcToDo + '] at ' + TIME())

    ENDIF

  NEXT 

  loErr = .oMig.GetErrors()

  IF loErr.nerrors > 0

     loErr.ShowErrors()

  ENDIF

ENDWITH

I have, deliberately, removed all the comments from this code block but it is possible to infer that this has something to do with a data migration. Contrast that with the code that is actually in my command button:

ThisForm.RunMigration()

See the difference? Now it is perfectly obvious what happens when this command button is clicked; a method named “RunMigration” on the parent form gets called!

The second reason for not placing code directly in the event is related to the first. In the example I gave here it is extremely unlikely that the RunMigration() method would ever be called from anywhere else except the one command button on a form. However, what about code that adds a new record to a table? Or the code to handle saving or reverting changes? Such code is completely generic (i.e. the code doesn’t depend on which table is selected when you are using commands and functions like  APPEND BLANK, TABLEUPDATE() or TABLEREVERT() – providing that it is the correct one).

There are two solutions that I often see in code. The first is simply that every form (and often every page on a form!) has its own SAVE button with code along these lines in it’s Click() event:

IF NOT TABLEUPDATE( 2, .F., ‘customer’)

  MESSAGEBOX( “Unable to save Changes”, 16, “System Failure” )

ENDIF

The second solution is where a generic “Save Button” class has been created which typically uses code along these lines:

*** We must be in the correct work area first

IF NOT EMPTY( ALIAS() )

  IF NOT TABLEUPDATE( 2, .F., ALIAS())

    MESSAGEBOX( “Unable to save Changes”, 16, “System Failure” )

  ENDIF

ENDIF

So again we have to ask ourselves where should this code go? Clearly the fewer places, the better so the first solution (simply writing the code directly in the Click() is not good). The idea of using a class is better, but the key question in this, as in so many other questions in OOP is “Where does the responsibility lie?”

To put it another way, is it really a command buttons job to handle updating a table? I would say definitely not! The function of a command button is to inform the system that the user want something done – but it doesn’t follow that the command button is the best place to actually DO whatever is required and the save code is a good case in point. Obviously the object that is ultimately responsible for saving data is the form, so doesn’t it make sense to put the code there? If all that is really is required is the simple code I showed above, then this can go into a single method in the form class and simply be inherited by every form based on that class.

Once the code is on the form we no longer need to worry about where it is being called from! Given the assumption that the calling object is going to be responsible for setting the work area anyway, we can call ThisForm.SaveData() from any control, anywhere, in a form AND still know what is going on when we revisit the code 6 months later.

The third reason for eschewing the location of code directly in an event, in favour of putting it in a method is related to the nature of methods and events. The fundamental difference between a method in VFP and an Event is actually in how and when they are called. A method must be called, explicitly, in code with a qualified reference to the object in which the method is defined, thus:

ThisForm.Refresh()

_Screen.ActiveForm.AddProperty( ‘junkprop’, NULL )

This is not the case for events. An event is raised as the result of some action and calling the method of the same name does not actually fire the event. Thus the code that executes when a command button is clicked may be fired in two ways:

·         Explicitly by calling the button’s Click() method in code. However merely running the code does not cause the event to fire

·         Implicitly as a result of the user clicking on the button, firing the click event and so executing the associated code

Perhaps the easiest way to illustrate this is with an event that is often used, but rarely called programmatically – KeyPress(). Drop a base class textbox on a form, and add the following to it’s KeyPress() event:

LPARAMETERS nKeyCode, nShiftAltCtrl

IF VARTYPE( nKeyCode ) = "N"

  WAIT "This was called as an EVENT" WINDOW

ELSE

  WAIT "This was called as a METHOD" WINDOW

ENDIF 

Now add a command button and in its click (yes, I know…) just add:

ThisForm.Text1.Value = "A"

ThisForm.Text1.KeyPress()

When you run the form you will see that typing the letter “A” into the textbox fires the KeyPress() event and you get the “Event” wait window. However, clicking on the button, even though it inserts the letter “A” into the textbox (and the result is indistinguishable from typing it yourself) the KeyPress() does not fire! Moreover, just calling the KeyPress() method executes the code that is there, but since the calling code does not pass the correct parameters, we see only the “Method” wait window.

Of course, we could simulate the event by passing the correct parameters explicitly but that is not the point here. The point is that the code in this event is always going to fire whenever the event occurs – whether we want it to or not! I picked the Keypress deliberately because I have often seen code like this in this event:

DO CASE
  CASE nKeyCode = 18 OR nKeyCode = 57 OR nKeyCode = 31 OR nKeyCode = 153 && Page Up

       *** Do something special here and eat the keystroke

    NODEFAULT  

  CASE nKeyCode = 3 OR nKeyCode = 51 OR nKeyCode = 30 OR nKeyCode = 161  && Page Dn

       *** Do something special here and eat the keystroke

    NODEFAULT  

  CASE nKeyCode = 5 && Up Arrow

       etc etc etc…

  OTHERWISE

       *** Not a navigation key – just process it

ENDCASE

This seems to me to be a very inefficient way to process keystrokes (remember, this code will fire on every keystroke)!  DO CASE is probably the worst possible choice for this sort of processing as it is relatively slow. A better solution might be to use an INLIST() function call and redirect execution to a method if the keycode is one of those that we are interested in. Thus the code could be re-written as:

IF INLIST( nKeyCode, 18,57,31,153,3,51,30,161,5 )

  ThisForm.HandleKeys( nKeyCode )

  NODEFAULT

ENDIF

Irrespective of whether it really is quicker, this still feels better to me, not only because it gets rid of the code from the event, but also because it is much easier to see what is happening. Quite clearly certain keys are being intercepted and passed off to some special handler code. Something that is not immediately obvious when a huge DO CASE statement is embedded in the KeyPress() of some control.

The additional benefit is that if we needed different forms to handle the same keys in different ways, we don’t actually need to change any code in our controls – just the form method itself. Much cleaner.

As always, there is no absolutely “right” (or “wrong”) way to do this sort of thing, but I am a firm believer in keeping it simple and to me, having my code under my own control where I know what it is doing and when it is called, is the simplest way. Since doing it this way also makes it easier to centralize and maintain my code, this one is a no-brainer for me.  But if you have ever found yourself writing code like this, maybe it's time to think again....

ThisForm.PageFrame1.Page1.Container1.OptionGroup1.Optionbutton1.Click()

Have fun!

Published Monday, May 30, 2005 10:50 PM by andykr