Poor Code vs. Good Code

[Elias]

Ok, here's the dispute, a collegue of mine and I disagree on whether or not it is poor programming style to call an event procedure from other procedures. The following is a simple example:

Corbin's Way!
--------------------
Dim intX As Integer

Private Sub Command1_Click()
intX = intX + 1
Text1.Text = intX
End Sub

Private Sub Form_Click()
Command1_Click
End Sub

Private Sub Form_Load()
intX = 0
End Sub


Eli's Way!
------------------------
Dim intX As Integer

Private Sub Command1_Click()
Increment
End Sub

Private Sub Form_Click()
Increment
End Sub

Private Sub Form_Load()
intX = 0
End Sub

Private Sub Increment()
intX = intX + 1
Text1.Text = intX
End Sub

Your comments will be greatly appreciated. Please be clear on who's code you would prefer and why. Thanks!

[Stevie]

I would always go for Eli's way.
I don't know if you would say its the proper style, but I always call subs/functions from events. I very rarely have more than one or two lines of code in an event (with the exception of the Form Load and UnLoad events).
I think the reason I prefer it is that you have a nice list of procedures in the Declarations combo box, which can all be meaningfully named.

[QWERTY]

I would use Corbin's code, because it is shorter. It has everything Eli's code has, but it is well basically shorter. But to be honest with you I don't think that any of these codes is bad or good. They are a little bit different, but it doesn't change anything in the way they work. If you look at these codes you'll see that they are so similar to each other, that for me it wouldn't make any difference if I had to choose one of them.

whether or not it is poor programming style to call an event procedure from other procedures

It depends on how many times you'll call the event. Corbin's case, he uses it only once. It would be OK with me. But if I had to call it more than 3 times I would rather use the Procedure (your way).

Just my $0.02

[PaulLewis]

I prefer Elia's code although I have certainly done both. Sometimes it is just quick and easy to do it Corbin's way so I do.

When I come back to tidy up, I create separate Functions or Subs as Eli does.

Reason:
If you are thinking in terms of a procedural language, then really, one procedure is as good as another, so the click "event" (just a Sub afterall) or the Increment Sub are as good as each other.

If you are thinking in terms of an OO language, then by adding to the event method of a control you are in effect changing the behaviour of that control (extending). By calling the procedure that an event triggers, you are completely circumventing the object model. VB lets you do this conveniently and so in that respect I do not believe it to be "bad programming", just not my personal choice...

If you could trigger the event, then this would be proper OO use of the control. For example, if you truly did trigger the click event, you would expect all the behaviour associated with the click action to occur, including the button being pressed and so on.

If corbin continues to do it his way exclusively, then should VB13 (ok that could be a ways off..hehe) come along and totally encapsulate a controls events making them private, then he will soon find that Elia's method is the only way to proceed. I needn't worry too much though since I doubt VB will do that to you. If it did, it would be a far different language I feel...

I like VB because you can have a foot in both camps. You are allowed to do what you do because today, VB is a procedural language with lots of OO features.

(I'm primarily a Java programmer by the way so my bias is toward OO...hehe)

Cheers
Paul Lewis

[Nitro]

I would use Corbin's method too because it is shorter. I agree with Qwerty!

For good coding, I also prefer to use the word CALL so that when you look into the code, you automatically knows right away it is calling another procedure and that you don't mistaken it for a method.

Plus, PROGRAMMING is like writing. Everyone have different style. I don't really think neither one is consider as poor codes in this case.

If you want to see poor codes, come and work with the contractors at my company. They are in love and only no how to use "IF" statements. They don't know how to use loops and "Case Select" and they consider themselves as programmers. Scary!

[Edited by Nitro on 07-06-2000 at 06:51 PM]

Shorter code reduces the introduction of bugs. Always use the CALL verb to make your code easier for some one else to read.

As Nitro said, neither is better than the other. Corbin's way is shorter, but when you get into larger projects, placing Subs and Functions in a Module would be a lot cleaner.

[HeSaidJoe]

I use both..it's a time of the day decision..no biggie.
both do the same thing and both are very traceable..
My preference is Corbin's way cause it seems more convenient...but I also agree with Megatron and use the Call Command1_Click instead of Command_Click


........of course it's only my 2 Cents worth..

[Sam Finch]

Personally I'm with Eli, especially with a command button called Command1.

I don't think there is any right way, If there is I'd have to go with Stevie about having all your code in functions, but with sensible command button names there is really no need.

I use lots of class modules so most of my command buttons just call methods in class modules etc. Unless it's throwaway code in which case I do whatever I feel like.

As Long as your code is reasnobly well commented so you can remember what's going on It doesn't matter.

Personally I don't use the Call KeyWord at all, it's probably a bad habit rather than anything else.

I'm 100% with Eli on this one.

I NEVER call event procedures from other procedures, it's very unstable.
More than that, creating a public function with an understandable name is MUCH MUCH easier to read and debug.

BTW,
Get some real names for your controls!

Just keep everything consistent throughout the project, keep reviewing and dump any code that's going to be called more than once into a procedure or bas module if ya want to use it in other projects.

In this game everyone has their own style of coding, therefore it's a real grey area as to which is the bast approach. Sam might write something one way, l might take a totally different approach, neither of us would be right or wrong. It would just be our styles.

[Ruxpin]

I prefer Elis code. Basically because, if another programmer was to come along and modify the code. They are more likely just to delete a click event thinking it wont impact on anything else rather then a seperate function.

[(B2F)Tom]

Corbin'code is shorter so, I prefer it. But three notices:

First of all, a button called Command1, there's better
Second of all, Look for is there is a possibility to avoid Command1 because you've got two differents events for the same action.
Third of all, calling Subs or Functions is great but for each Event, I think the one who vill read your code doesn't want to be shacking between Modules and Forms...

My opinion of course!

[benski]

I reckon this is gonna be a hot topic soon...

Corbin's way-
what I tend to do if I'm being lazy or if I don't realise that I am going to refer to what's in the event procedure many times.

Eli's way-
if I know that I'm going to need to use the increment code in many places, or later in development I want to tidy code for legibility.

Verdict-
It has to be Eli's way- for pure legibility

[Paul282]

Elies Code!

Jethro... ??????????? You aggreed with two people who agreed with opposite side of the argurment! which one do you like?

There's nothing wrong with Corbins way in a real sense, but if I had to support another persons code I'd prefer the non-spaghetti method. Shorter code is rarely better code, it's usually lazy code.

[Mark Sreeves]

Jethro says:

Always use the CALL verb to make your code easier for some one else to read.

well I disagree with that totally!

Call is optional and I NEVER use it!

Obviously if the "house style" is to use Call and you are in a team of programmers, then you should.


Regarding Elias's original question, I reackon a separate procedure is better but if (for example) you wanted a timer to click a button then calling the event code would be clearer.

Private Sub Timer1_Timer()
  Command1_Click
End Sub

[Crazy D]

That's your way of coding, but I think you can't disagree with the fact that when subs are called with Call it immediately shows it's a sub that's beging called. On the other hand, everyone knows that the only thing that can be on 1 line with nothing else is calling a sub, since when you just put a varname on one line without anything else it will give you an error.
I usually use seperate subs/fucnctions if it's being called from a command click event, and from another place. Just to make it easier for myself, since I usually write the sub before I put the call in the event where it should be called, and 2nd, I've made enough mistakes by deleting a command_click event just because I forgot it was pretty important code (yes, that's stupid, but reality).
Oh and what's wrong with being lazy? Good programmers are lazy imho, they don't do something that's not needed, and if a certain code is used more then once, put it in a seperate sub so if there has to be changed something in there, you only have to modify it once. So lazy is good :-)
But most important is that code has to be readable, preferably with some useful comments so other people (or yourself a couple of months later) still understand what's going on. I've worked in another company, had to write custom solutions, based on code that was used and ported since 1985 or so (was C-code), which was not documented at all (yeah, "this is a var to save the date" (and the var was called TheDate... useless...), and even though the code itself was written pretty well, it's always easier to read comments to walk thru code, then to read the code, and try to understand what cvertain functions do (ok here comes the naming convention,if you use usefull describing names that helps a lot).
But at the end, unless you write in a team wherer you have to use coding standarts, use what you like the best, what's the easiest for you. At the final end, the end-product is what it's about, and if the product does what it's supposed to do, it's good code :-)

[kedaman]

Well I would agree with Eli if I wanted to give my code to somone, since it's more professional to structurize your code hierarcally, anyway i have used Corbin's oftenly by myself, ie when I call short code from toolbar code to menu code. I think it depends what purpose you have for the code.

Personally I prefer the shortest code, but sometimes I have to disagree with myself to obtain the fastest code, i.e. if i make my code inline.

Eli's example is not particulary good to show why (I would have a very frequently used code) but here's what i mean:

Dim intX As Integer 

Private Sub Command1_Click() 
intX = intX + 1 
Text1.Text = intX 
End Sub 

Private Sub Form_Click() 
intX = intX + 1 
Text1.Text = intX 
End Sub

[Paul282]

In my copy(s) of VB, if I don't use Call when I add params to a sub I get a runtime error. and the design time is odd.

I like that!

Also according to MSDN...

You are not required to use the Call keyword when calling a procedure. However, if you use the Call keyword to call a procedure that requires arguments, argumentlist must be enclosed in parentheses. If you omit the Call keyword, you also must omit the parentheses around argumentlist. If you use either Call syntax to call any intrinsic or user-defined function, the function's return value is discarded.

So I use it

[benski]

Yeah, that is just saying if you have a sub:

Sub DoSomething(Something As String)
'something...
End Sub

then you can call it with either

Call DoSomething("Something")

or

DoSomething "Something"

[Iain17]

The reason you get an error.

You need to call the sub without placing the arguments inside brackets.

Private Sub mySub (arg1, arg2)

End Sub


'works
Call mySub (1,2)

'works
mySub 1, 2


'fails
mySub (1,2)

[benski]

wasn't that what i just said???

[RogerH]

I almost fully agree with Eli's code. I'd even avoid the global variable by using a static:

Private Sub Command1_Click() 
Call Increment 
End Sub 

Private Sub Form_Click() 
Call Increment 
End Sub 

Private Sub Form_Load() 
Call Increment(True)
End Sub 

Private Sub Increment(Optional Init as boolean) 
static intX As Integer 

if Init Then
   intX = 0
else
   intX = intX + 1 
endif

Text1.Text = intX 
End Sub

Roger

[Paul282]

Is this thread going anywhere?

If you're for Corbin's Way you're a loser!

By-gones!

No point us all just agreeing with eachother, If you think otherwise... State your case!

(And if your claim is "short code" ... Death is too good for you!)

[kedaman]

You're probably not a loser at all, but you will probably lose if you don't care about the possibilities...

Personally I think it's a matter of programming style, you do as you wish to do.

If you want actual difference, do something that actually have some effects as compiled

As I said before, I vote for neither. Corbin's code is shorter, but in my opinion, it's disorganized.

Paul

I preferred one method which had shorter code, (remember less chance of introducing bugs), however l don't really have a problem with longer code if it is correct. Therefore it comes down to style. Personnally l feel that if you can achieve something with a minimum number of lines you are writing eloquent code.

Mark

Once again this is style. We always force the use of the CALL keyword because it makes the code easier to read. Have you ever had to support another guy's code? Probably not, else you would understand that even though the keyword is not needed it makes it simplier to track bugs/mods through complex projects.

Megatron

Give a guy "guru" status and look what happens Only kidding, you are right with what you say regarding shorter code but disorganised. It's a dangerous sign when names like Text1.Text appear in code blocks.

[jmatello]

In my opinion Eli's way is the way to go for the following reasons:

1) In a small program, either way will work. But what if the project becomes huge and you use Corbin's method all over the place...what a maintenance nightmare.

2) Like many others have said...when you see Call bla bla bla, it's easier to follow and understand the code.

3) My biggest factor...if you call the Command1_click event from Form1_click event, what happens later if you decide to add code to the Command1_click event that you only want executed if the user clicks the Command1 button, you have to set a flag or something to avoid the code from being executed when the Form1_click event fires. What a CF this could become!!

[Sam Finch]

Jethro, When you say force the call method do you mean there's an option you can set which mens you have to use it or you just go around with a big stick and give anyone who doesn't a good bruising?

Basically we have set coding standards that everyone complies with, (at this stage all three of us). Using the "Call" keyword is just one of them, but it makes the code easier to read over multiple projects.

[Serge]

I always try to create a generic routine, so I would go with Elias' way, but I would modify Increment function to be more generic, since this routine can be used by more then 1 TextBox (or atleast I assume that it can be):

Option Explicit
Private intX As Integer

Private Sub Command1_Click()
    Increment
End Sub

Private Sub Form_Click()
    Increment Text1
End Sub

Private Sub Form_Load()
    intX = 0
End Sub

Public Sub Increment(pTextBox As TextBox)
    intX = intX + 1
    pTextBox.Text = intX
End Sub