Keeping code "clean"

[smUX]

Okay, this is not so much a VB6-specific post (pretty much all languages would benefit) nor is it a question, but I felt it needed posting and as I am a VB6 coder I should post it here rather than another section (mods, feel free to move it if it should be elsewhere).

I don't consider myself to be a great coder, I'm only amateur, but I know a fair amount of tricks that help me improve my own code and I thought I'd explain for a number of other people who post here.

A lot of people post their code here and I notice each time that their code is hard to read, and sometimes this is part of the problem...if you can't read your code without carefully looking at each line, you're sometimes going to miss a problem that might normally jump out at you.

Note that the code I am using as an example is JUST an example...it wouldn't really do anything, and it could be done better anyway...and I know it could

Indenting your code is one way:

Private Sub Form_Load()
For b = 1 To 1000
For c = 1 To 1000
DoEvents
Next c
Next b
End Sub

If this was indented, it'd look like:

Private Sub Form_Load()
    For b = 1 To 1000
        For c = 1 To 1000
          DoEvents
       Next c
    Next b
End Sub

Straight away, you can see the flow of the code, you can see that there's a for/next and inside it is another for/next and inside that is some code that does something. However, there's more you can do to help you with readability. In this case we're assuming that this small section of code is initialising a global variable, so why not put the code into a function and store it in a module?

Private Function init()
    For b = 1 To 1000
        For c = 1 To 1000
          DoEvents
       Next c
    Next b
End Function

Now, in your Form_Load you just put "init" and it'll run the code in the function...you've turned the Form_Load from 5 lines of code into one line and the name of the function tells you what it's doing. If you need to add to the init section of your code, you can go to the module and do the work.

Many people use tricks like this to simplify their code, and there's a good reason...it makes your life a hell of a lot easier. Also, if you have a lot of form initialisation (label captions, locations, etc) you can do the same thing from a function in a module. If you had (for instance) a label called label1 on form1 you would change the caption from Form_Load with label1.caption = "Hello" but in a module you'd put form1.label1.caption = "Hello". It's this one minor difference that you need to remember when using module functions to affect your form elements.

This suggestion doesn't sound like much, and it may even sound like more work and perhaps a larger resulting code/exe, but when it comes to reading through your code when you have a bug (or even when you want to add something to a certain section of your code) you'll find it saves tons of time...functions aren't JUST for bits of code you might want to reuse elsewhere! If you have lots of small clips of code that you use in a sub that can be split up into multiple functions, storing them in a function would improve readability by a ton...you can imagine a 100+ Form_Load sub which is reduced down to 5 lines of code which describe exactly what that sub is doing (and no, commenting the code doesn't help as much as you might think, although you should still comment your functions for readibility as well) and if you need to find a specific section of the Form_Load you should know from memory what you called it and can go look at the function you want...you can even split modules up by section or by whatever sorting method you want to use.

Hope this helps the way it helps me when I'm coding

[CDRIVE]

Actually for good readability there should be an indent at the beginning and end of a sub or function.

This...

Private Sub Form_Load()
For b = 1 To 1000
    For c = 1 To 1000
        DoEvents
    Next c
Next b
End Sub

should look like this...

Private Sub Form_Load()
   For b = 1 To 1000
       For c = 1 To 1000
           DoEvents
       Next c
   Next b
End Sub

[smUX]

Good point, I'll make changes to the post (and add this here so people know I've done it based on your suggestion )

[Pradeep1210]

NEVER put your code in a module unless you want it to be accessible from everywhere in your entire application!!! Also if you code it in the right way, you should never have the need to reference a specific form or form element like form1.label1.caption = "Hello" from a module.
If you do that you eliminate the difference between PRIVATE and PUBLIC scoping.

What you suggest might work and also may look cleaner to you. But one silly mistake and you would be doomed! It would be way too hard to debug that code than you could imagine.

[smUX]

Having code accessible from anywhere in the program isn't a problem if you're going to access it at least once, which you are when you use it in the parent sub (where you're calling it from) and it shouldn't be difficult to debug if you know the basics of VB6...functions are only called when YOU call them so if you call a one-time function more than once it's your fault really :-)

[Pradeep1210]

No, you don't get the point really.
What you say might work for small applications. But see it like a large application, where many programmers are coding parts of it simultaneously. See how much mess it would be if you make available to one programmer what he should not be even least bothered about.

Even when a single programmer is coding, it won't be a smaller mess!
Assume you have 20 forms in your application and each form has about 20 controls on it. Each control on an average has say 10 event handler functions. So your module will have 20*20*10 subs?? And how do you distinguish form1's init from form2's init? Yould have 20 inits (with different names) and you would have to choose from the list. Same way if the total count of all buttons in your application goes to about 100, you need to choose from 100 functions, which one to call.

So there is no point in making things global unless they really do something global.

[smUX]

You answered your own point about distinguishing form1's init from form2's init...form1_init, form2_init, etc...although I take your point that it may look messier...but if the code you're replacing with a function is a specific section of code that you know the function of (like setting certain global variables or loading a text file into memory, etc) then you can move that code out easily and replace it with a named function to give you info on what code is being called at that point.

This suggestion is probably more useful for smaller programs, as the amount of modules could become huge, but if the modules are properly named it makes it a whole lot easier to manage...I guess my point is don't knock it until you've tried it, because I have never had a problem with working this way, bugs have never been harder to track down, perhaps they're even easier because the code is easier to read :-)

If multiple programmers are working on the project, it might be a problem if they don't all understand the layout and how the functions are being used, but isn't this how multi-programmer projects are anyway? Perhaps if all the programmers are briefed (I'd assume they are, anyway) on what's being done in the code, it wouldn't be a problem.

This is no different to using a DLL only once in a program...rather than calling a DLL you're calling a function...if you see what I mean :-)

[Pradeep1210]

You answered your own point about distinguishing form1's init from form2's init...form1_init, form2_init, etc...

So what difference does it make if it is called Form1_Load somewhere or Form1_init elsewhere?

[smUX]

So what difference does it make if it is called Form1_Load somewhere or Form1_init elsewhere?

You didn't read the post fully then...moving unimportant code into a module makes it easier to read which is the whole idea behind the method I use. Rather than your Form_Load event (or whatever part of your code you're working with, it isn't limited to Form_Load) being bloated with tons of code you have the code stored in an easily accessible block in the module...if you need to quickly find the init section for form1 you can just go to Form1_init in the module section and make the changes, and if you don't need Form1_init and you're looking through the Form_Load section of form1 you don't have to scroll through form1_init code to find what you're looking for...I guess all I can say is to reiterate what I have said before...don't knock it until you've tried it...it may seem alien to you but that doesn't mean it's wrong, and your word NEVER is definitely wrong.

[techgnome]

just because someting isn't wrong, doesn't mean it's right either.
Consider the form1.label1.caption ="Some default value" line. Here's what isn't correct about it: It uses the default instance of the form - which may not be what I'm trying to load. What if I just made a new instance of form1? Now the code in the form1_init isn't going to work. Now I have to rewrite the sub so that I can pass in a form so that the label can be set correctly. Now, I have to rewrite the code that created the form to call another function, pass in the form I jsut created. OH! Then I decide I could use this form in another project... so I copy and paste it to the new project and then have to go back and figure out all the other dependencies that I have to copy over just to get the form to work.

Or... the form could be self-contained and all contained in itself. I'm not saying put everyting back in the Form_Load event.... I just question whether init needs to be in a module.

-tg

[smUX]

Why is it two people have concentrated on this Form_init? That was an example, and one of many ways in which I've used the code.

Also, form1.label1.caption is *again* an example. Do you put label1.caption in your code after renaming label1 to something more meaningful? No, you rename it, it isn't rocket science. When you make changes in your form, you modify your module just like you would modify your main code normally, in that case you would know exactly where to go to find what to change and rather than having to go through 100s of lines of code you'd be able to find the right section of the module and edit it accordingly.

Also I should point out that I am not trying to say it is right, but I am saying that it isn't wrong...there's many ways to do the same thing in VB6, that's the beauty of it, and this is just my method for keeping my main subs clean and understandable...it has no detrimental effect to the running of the code and once you're used to the method it has little or no detrimental effect on your coding and can instead HELP you with organising your code :-P

[LaVolpe]

I can understand both arguments. There may be cases where a module or class is written to change/modify a form's controls (i.e., a resizer). But with those cases, it is better to pass a reference of the object to the module/class, i.e.,
Public Sub UpdateLabel(theLabel As VB.Label)

Now the code is still generic, because the user simply passes: Me.Label1 to the sub.

Edited. Sorry, we posted nearly same time; I see you want to focus attention elsewhere

[smUX]

Here's an example of something I wrote for someone here...check out the form_load in the file at http://www.vbforums.com/showpost.php...0&postcount=14

Ignoring the comments, it's 3 lines:

Private Sub Form_Load()
'This randomises the questions by randomising the seed generator in VB
Randomize
'This line calls the code that loads the questions into memory
setquestions
 
'Now to add a new question to the board
newquestion
 
End Sub

Now here's the same form_load without setquestions (newquestion has to be there as a sub because it is called elsewhere to set a new question)

Private Sub Form_Load()
'This randomises the questions by randomising the seed generator in VB
Randomize
 
'Here you will see the questions being set...they're put into memory here, to be called
'later on as required
strQuestion(1) = "When was pakistan first made?"
strAnswer(1, 1) = "1947"
strAnswer(1, 2) = "1961"
strAnswer(1, 3) = "1945"
strAnswer(1, 4) = "1955"
strQuestion(2) = "Who comes on christmas day?"
strAnswer(2, 1) = "Santa claus"
strAnswer(2, 2) = "Santa flaws"
strAnswer(2, 3) = "Santa Ben"
strAnswer(2, 4) = "Santa Jonas"
strQuestion(3) = "What is the most common substance found on the planet earth?"
strAnswer(3, 1) = "Water"
strAnswer(3, 2) = "Gold"
strAnswer(3, 3) = "Diamond"
strAnswer(3, 4) = "Humans"
strQuestion(4) = "Who was the leader of the Nazi party in Germany?"
strAnswer(4, 1) = "Adolf Hitler"
strAnswer(4, 2) = "Joseph Stalin"
strAnswer(4, 3) = "Field-Marshal Rommel"
strAnswer(4, 4) = "Himmler"
 
'This one tallies to how many questions you have in your database (above)
dblMaxQuestions = 4
 
'Now to add a new question to the board
newquestion
 
End Sub

Hopefully you'll better realise what I am trying to say...3 lines of code with 3 meaningful commands, two of which are custom and named for simple understanding by the person reading the code

[Merri]

Personally, if it isn't a really long of block of code, I'd rather see the code where it belongs rather than trying to locate a procedure. I expect to find initialization code in Form_Load. If you add extra procedures just for the sake of having a very short code in Form_Load it adds extra work for me when I'm reading the code and trying to understand how it actually works. It gets worse when there are more procedures built like this. Each time I have to jump around a multitude of procedures I feel like rewriting the whole thing I'm dealing with.

For a good sample I can pick cSocketMaster: being written the way it is, it inspired me to write UniSock. However, UniSock failed as I chose to use wrong subclassing thunk. I should have used the same used by cSocketMaster. Anyway, I find UniSock's code much easier to read, being streamlined and using extra helper procedures only when they are really needed. You don't need to jump back'n'forth the entire thing just to understand what is going on (assuming you are good enough to understand it in the first place).

[smUX]

Again with the form_load...that was just an example...it could be anywhere, like in a button...you could have a button which loads and parses data, and you could have a "load_data" and a "parse_data" module function set up to handle it...and in the button's code you can see it loads data then parses data, and could look into the "load_data" module if there's something up with the data loading, or same with the "parse_data" code.

If you're the one who wrote the code and you're not working on multiple projects, you should find it a simple enough process to read through your code...plus if you KNOW the load_data section is working fine you can ignore one line rather than ignoring the whole block of code that deals with the data loading...there's a whole tons of uses, although some people aren't used to working like that or are unable to adapt :-)

[Merri]

Now you are yourself taking too much on Form_Load: all what I said is generally true for everything (in my view of reading & understanding code).

[vb5prgrmr]

Actually for good readability there should be an indent at the beginning and end of a sub or function.

This...

Private Sub Form_Load()
For b = 1 To 1000
    For c = 1 To 1000
        DoEvents
    Next c
Next b
End Sub

should look like this...

Private Sub Form_Load()
   For b = 1 To 1000
       For c = 1 To 1000
           DoEvents
       Next c
   Next b
End Sub

I don't agree with this especially if you add error handling... Because, while you can indent the On Error GoTo line, you cannot indent the anchor...

Private Sub Form_Load()

  On Error GoTo Form_LoadError
  
Set B = Controls.Add("VB.CommandButton", "Button")
B.Width = 1500
B.Height = 750
B.Left = 30
B.Top = 30
B.Caption = "Test"
B.Visible = True

Form_LoadError: 'cannot indent to match up with the on error goto line

End Sub

and so this breaks the "indenting rule". AND if you have some really long lines of code, you are also reducing the number of characters that will print per line when it comes to printing out the code. BUT PLEASE do not get me wrong about indenting code, I believe in it it, just not the part about indenting the first line of each procedure whether it is an on error goto or a for next or if or select case or ...

I also believe, that if you can break something out of a procedure, one should do so especially if that piece of code is/can be used by other procedures and/or if you are trying to make your code more "readable". For example...

Private/Public Sub/Function MySub/FunctionName...

'something happens here

'and something happens here but it is so long or complex you break it out into another procedure
HereWeCallSomeOtherProcedure

'and finally we finish up here with the results from the other procedure

End Sub/Function

Private Sub/Function BrokenOutCodeHere(RightBelowWhereItHasBeenBrokenOutOf)

'do the complex thing here

End Sub/Function

So that to "find" the broken out procedure, alls you need to do is hold the CTRL key and press the down arrow key or press Shift+F2

But hey, coding styles are like opinions, everyone has one...

[Pradeep1210]

I can't stress on this any more:

NEVER give your variabes/functions a scope more than what is really needed.

And since things in modules are public, it implies that "don't put anything in a module that shouldn't really be available publicly."

Declare the variables/functions as close to the place they would be used and let them go out of scope as soon as you need them no longer. I know languages like VB6 doesn't support a scope smaller than the sub/function, but most modern languages (including .net) support scoping to the level of a block (e.g. within If.. End If etc.). So for VB6 it is customary to declare them just as the first few lines inside the the sub/function. As a rule, never move them outside the sub/function at the form/class level or into a module unless you really need them to be accessible from somewhere else too. And the same rule applies to subs and functions: Never make them public unless you really want them to be public.
This is just one of the underlying principles of OOP concept.

Also, for what you say shrinking the code blocks to make them readable, you should be better off putting those sub/functions inside the same form and declared as PRIVATE. Why do you need to move it to a module and expose it to the entire application??

[Hack]

Okay, this is not so much a VB6-specific post (pretty much all languages would benefit) nor is it a question

And for those reasons, I'm moving it to General Developer.

Good topic!

[smUX]

Also, for what you say shrinking the code blocks to make them readable, you should be better off putting those sub/functions inside the same form and declared as PRIVATE. Why do you need to move it to a module and expose it to the entire application??

I'm not going to "argue" too hard against that, although personally the idea for me is to move the data out of the form's code window completely to make it so there's less lines visible when debugging. I suppose if you added a load of lines between the end of the actual code for the program and the subs/functions made for readability then it might work out better than making those subs/functions public but I also guess that it depends on what code is in the subs/functions...if it's referencing the form, it'd definitely be better in the same form data so you don't have to reference the form...I guess it's whatever you feel comfortable with.

[CDRIVE]

I don't agree with this especially if you add error handling... Because, while you can indent the On Error GoTo line, you cannot indent the anchor...

Private Sub Form_Load()

  On Error GoTo Form_LoadError
  
Set B = Controls.Add("VB.CommandButton", "Button")
B.Width = 1500
B.Height = 750
B.Left = 30
B.Top = 30
B.Caption = "Test"
B.Visible = True

Form_LoadError: 'cannot indent to match up with the on error goto line

End Sub

I guess its a matter of what one considers good coding practice. I would write your code like this, which I find far more pleasing to the eye and easily readable.

Option Explicit
Private WithEvents cmdMyButton As CommandButton

Private Sub Form_Load()
   On Error GoTo ErrorTrap
      Set cmdMyButton = Controls.Add("VB.CommandButton", "cmdMyButton")
         With cmdMyButton
            cmdMyButton.Width = 1500
            cmdMyButton.Height = 750
            cmdMyButton.Left = 30
            cmdMyButton.Top = 30
            cmdMyButton.Caption = "Test"
            cmdMyButton.Visible = True
         End With
      Exit Sub
ErrorTrap:
   MsgBox "An unknown error has occurred!"
End Sub

[dilettante]

One thing I haven't seen discussed yet is the cost of using static (.BAS) modules.

These get loaded when the program initializes and stay in memory for the life of the program. This is why things like callbacks have to be in static modules.

If a static module only holds code and data used in a Form or two, and the life of the program is long, you're just spending memory you don't have to. The Form or Forms can be unloaded, but all of this other code and data sits there until the program ends.

If you're trying to factor out code and data for maintainability and reuse you might want to consider using more Classes and UserControls. Much of the reason for classes in general is information hiding, i.e. getting the details out of the point of use. Classes (or their instances, objects) are dynamic, and can be unloaded unlike static code and data.


There is nothing in VB called an anchor, I think you meant to say line label.

[Shaggy Hiker]

So much of this debate would go away in .NET. When I worked in VB6, I tended to do something like this to organize the code. I wasn't working in a team environment, nor would I ever be on those projects, so the scoping rules mattered less than efficiency and organization. However once moved to .NET, the whole point becomes moot. Regions allow you to collapse away any methods or data members that you don't want to see. The more strict adherence to OO principles meant that modules are much less valuable, and should be specifically targeted. The init method, if there is one (and there are plenty of reasons to have one), should be a member of the class because it is logically a member of the class. Moving it out into a module would be saying, in effect, "this object can't exist without the prior existence of this other object (a module is a class)." That's a pretty bad principle, but not so much in VB6 where OO prinicples are generally deficient at best.

[Shaggy Hiker]

There is nothing in VB called an anchor .

Yeah there is: VB6.

After all, 2010 will be out in a couple months and look how many people are still working in VB6. THAT's an anchor!

[smUX]

One thing I haven't seen discussed yet is the cost of using static (.BAS) modules.

These get loaded when the program initializes and stay in memory for the life of the program. This is why things like callbacks have to be in static modules.

I have to say that's a very good point...memory has to be a consideration when using this, and if you have a lot of these functions you can either keep them in the same form (which kinda defeats the purpose a little, but is workable) or not use the method. For the most part, people who would use this program are amateur or semi-professional coders who might need that little bit of extra help with legibility of their code rather than the professionals who've already got their own way of working or learned another and it'd be hard to switch...amateur programmers probably won't need to worry too much about the extra memory overheads or the globalisation of the code as long as they give the functions meaningful names that have no chance of being re-used accidentally or anything, but semi-professionals might prefer to keep the code in the same form (but not in the same event...set as a private function) as pradeep1210 mentioned previously.

[dilettante]

So much of this debate would go away in .NET. When I worked in VB6, I tended to do something like this to organize the code. I wasn't working in a team environment, nor would I ever be on those projects, so the scoping rules mattered less than efficiency and organization. However once moved to .NET, the whole point becomes moot. Regions allow you to collapse away any methods or data members that you don't want to see. The more strict adherence to OO principles meant that modules are much less valuable, and should be specifically targeted. The init method, if there is one (and there are plenty of reasons to have one), should be a member of the class because it is logically a member of the class. Moving it out into a module would be saying, in effect, "this object can't exist without the prior existence of this other object (a module is a class)." That's a pretty bad principle, but not so much in VB6 where OO prinicples are generally deficient at best.

Wow.

Starting with the last point, this is simply argumentative. The differences in the way languages implement "object orientation" are a reflection of the underlying plumbing. There is no such thing as a lofty "ideal" OOP and every language and component architecture does things differently. Look at the OOP-warts in JavaScript, or Java, or in .Net languages, or even in Object Oriented Cobol.

The regions thing in .Net is more of an IDE feature than a language feature. There is no question that it can be handy at times though.

I often wonder what amazing things we'd see in VB (I mean real VB) if Microsoft had continued creating new versions in the series. If you had ever used VB 1.0 (or even 3.0) you'd appreciate the amount of improvement VB 6.0 represented.

VB6 isn't the problem, a lack of a real VB7, 8, 9, etc. is the problem. There is no going back now though.


I think the reason this issue of so much use of static code exists in VB6 has much less to do with how "OOPy" it is than how often people fail to make good use of the "OOPy" features it does have.

[Shaggy Hiker]

OOP is a fairly well understood philosophy. Each OOP language does implement the core features in one form or another, and with no standard, that is all that can be expected...and probably a good thing. For instance, VB.NET doesn't really do multiple inheritance (though you can get the benefits through interfaces), but since multiple inheritance can cause so many problems, it is hard to say that not having it is a bad thing. It's not so much the underlying plumbing as it is the biases of the folks who developed the language.

Regions are ENTIRELY an IDE construct. They have no impact on the code in any way. However, you did talk about code organization as the impetus for some of your choices, and regions are all about code organization. By using them and partial classes, you can stay within a single class and have it organized however you want. If you want the member variables in one file, while private methods are in a different file, and event handlers in a third, you can do that. In my case, I often do that very thing, except using regions and having them all in one file. Organization is important in big projects, regions and partial classes just give you more options. I have no doubt that regions would have made it into the next version of classic vb, had it ever come out (it was in the works, at one point).

As for the problem not being VB6, I disagree. VB still exists, it just changed to .NET. After all, what is a language? Any language is just a syntactical means to make it easier to write machine code. VB6 and VB.NET use the same syntax, but VB6 wasn't designed to handle a few things that really needed to be handled. Multi-threading was barely possible under VB6, and it could only be done with contortions, as it was not actively supported. Object Oriented design wasn't really supported at all in VB6. You could make object like constructs (heck, modules were object like constructs), but you didn't have inheritance or polymorphism. VB6 wasn't structured in that way. Could those have been overcome in later releases...yeah, they were, it's called .NET. The biggest single problem for people moving up is that everything is now an object in a heirarchy of objects, a feature which is fundamental to OO languages, but didn't exist in VB6. So would a block structured language exist simultaneously with an OO language? Would MS maintain a C to go along with C++? It is certainly conceivable, but it wouldn't have been VB6, because the changes necessary to work with the framework would have been fairly profound in their own right. On the other hand, C exists because there is an international standard for it, and it has a very solid niche in the hardware. Old style VB has neither. What would be the incentive for a single company, even one as large as MS, to maintain two different languages that competed only against each other?

[techgnome]

The differences in the way languages implement "object orientation" are a reflection of the underlying plumbing.

I often wonder what amazing things we'd see in VB (I mean real VB) if Microsoft had continued creating new versions in the series. If you had ever used VB 1.0 (or even 3.0) you'd appreciate the amount of improvement VB 6.0 represented.

VB6 isn't the problem, a lack of a real VB7, 8, 9, etc. is the problem. There is no going back now though.

Why can't people see VB.NET as the natural progression of the language? What about VB2003 doesn't mean it isn't VB7? Making the leap that it did into true-er OOP seems like a natural progression from VB6...

It would seem you answered your own question there. The plumbing on the back end didn't allow for easy implementation of more OO features. It required some serious reworking. Enough so. that it was easier, better (arguably) and keeps VB more stable by re-building it using .NET.

-tg

[DeanMc]

I second this. People aregue that it is not true VB6 because there are breaking changes, yet .Net2.0 broke 1.1 and there where not half as many giving out. I dunno..

[techgnome]

VB3->VB4 was another break. VBX were no longer in vogue, and unless you remembered to save your VB4 forms as "text" forget about trying to open them in VB4.

-tg

[Nightwalker83]

The programs such as Visual Basic, etc have an option to set out code with indenting, etc so if you have the option enabled there shouldn't be a problem. Although, it has been ages since I have touched vb but it should do it automatically other tools such as Dream Weaver do.