Thoughts on practices

[kdbarrett]

Hi, I'm hoping to get some general feedback on coding practices I'm encountering. First, some background. I've been a developer for about 15 years, mostly contracting, and have worked in various technologies, languages, etc. VB, C#, SQL, Crystal Reporting, etc. As with many contractors, I'm a "jack of all trades, master of none" as the saying goes.

I'm currently working with a client's VB.Net code, which was developed for them by a small development shop a few years ago and which they purchased and have been maintaining and uprgrading since. This client's primary developer is out on indefinite (likely permanent) medical leave and I'm now filling in until they bring in a full timer (as I'm a contractor here).

My current task is to add some functionality to a the VB.Net code they purchased. I'm finding practices and techniques in the code that absolutely baffle me and can't make the code do what I want. I'm starting to wonder if it's me and was hoping to get some thoughts on the code I've encountered. For example:

Setting a variable to accept the result of a function by calling the function with many parameters, clearing the parameters in the function, setting them to some value, calling another function with those new values, then never using the values returned by the functions.

I'll add a code snippet in the first comment since this is already getting long. I appreciate anyone's patience in slogging through this...

 

[kdbarrett]

sValue = FunctionOne(sCol1, sCol2, sCol3, sCol4)
 
Private Function FunctionOne (ByRef sCol1 as String, ByRef sCol2 as String, ByRef sCol3 as String, ByRef sCol4 as String)
  sCol1 = ""
  sCol2 = ""
  sCol3 = ""
  sCol4 = ""
 
  If APreviouslyDeterminedValue = "something" then
    sCol1 = "a"
    sCol2 = "b"
    sCol3 = "c"
    sCol4 = "d"
  Else
    sCol1 = "z"
    sCol2 = "x"
    sCol3 = "y"
    sCol4 = "w"
  End If
 
  FunctionOneSub(sCol1, sCol2, sCol3, sCol4)
End Function

In the actual code "FunctionOne" might call 3 or 4 subs and/or functions with names like FunctionOneSub1, FunctionOneSub2, etc. The "FunctionOne" part being the only part that's made up here. When functions are used, the value is sometimes not returned or if it is, often not used in any way.

Am I nuts or is that just wrong?

 

[kdbarrett]

Also, I see this a lot:

<do some stuff>
[SIZE=2]System.Windows.Forms.Application.DoEvents()[/SIZE]
[SIZE=2]Sleep(200) [/SIZE][SIZE=2][COLOR=#008000][SIZE=2][COLOR=#008000]' sleep[/COLOR][/SIZE]
[/COLOR][/SIZE][SIZE=2]System.Windows.Forms.Application.DoEvents()[/SIZE]
[SIZE=2]<do more stuff>[/SIZE]

and

[SIZE=2]      <doing stuff in a nested if block>[/SIZE]
[SIZE=2]      System.Windows.Forms.Application.DoEvents()[/SIZE]
[SIZE=2]      System.Windows.Forms.Application.DoEvents()[/SIZE]
[SIZE=2]      System.Windows.Forms.Application.DoEvents()[/SIZE]
[SIZE=2]    End If[/SIZE]
[SIZE=2]    <doing stuff one level up>[/SIZE]
[SIZE=2]    System.Windows.Forms.Application.DoEvents()[/SIZE]
[SIZE=2]    System.Windows.Forms.Application.DoEvents()[/SIZE]
[SIZE=2]    System.Windows.Forms.Application.DoEvents()[/SIZE]
[SIZE=2]  End If[/SIZE]
[SIZE=2]  <doing stuff one level up>[/SIZE]
[SIZE=2]  System.Windows.Forms.Application.DoEvents()[/SIZE]
[SIZE=2]  System.Windows.Forms.Application.DoEvents()[/SIZE]
[SIZE=2]  System.Windows.Forms.Application.DoEvents()[/SIZE]
[SIZE=2]End If[/SIZE]

Why on Earth would you call DoEvents 3 times like that?

 

[JohnH]

Maybe you can learn something there.

It is not wrong, but since there is no return value it should not indicate that with the Function declaration, and instead declare it a Sub. A Better practice would be to not use out-parameters and declare a function that returns a composite object.

DoEvents (and Sleep) in UI thread is mostly done when the task should really be implemented as multi-threaded. Or perhaps in some cases using a Timer.
Calling DoEvents repeatedly? I have no idea why that is done, perhaps pay-per-code-line?

 

[kdbarrett]

Thanks for replying. Yeah, I suppose the function calling isn't really "wrong", but when I'm trying to understand the code and find myself chasing variables that end up having no meaning, it kind of feels that way.

That was my reaction to the DoEvents thing: they were getting paid by the line. Or someone who just doesn't understand DoEvents and thinks if you use it once, it speeds up the code, so if you use it three times it'll be even faster!

 

[kdbarrett]

Almost forgot to mention: some procedures are literally thousands of lines long. The biggest one is 3,700 lines of code. In one function. The program takes an input XML file, reads it and reformats it to the inhouse format, then reads from that to combine the data with PDF form templates and produce completed documents. Each and every field is hard-coded somewhere in those gigantic functions...

 

[JohnH]

Sounds like coded by someone with no concept of object oriented programming. Most of it can probably be heavily refactored to sensible objects and methods.

 

[kdbarrett]

That's the plan: replace this thing with oop. But here's something that occurred to me regarding DoEvents: the purpose of that is to get Windows to process any gui-related events, correct? Mouse clicks, window moves, etc. This programs is written as a service with no gui component at all. So is there any reason to EVER call DoEvents? Let alone three times in a row...?

 

[JohnH]

regarding DoEvents: the purpose of that is to get Windows to process any gui-related events, correct?

Yes.

This programs is written as a service with no gui component at all. So is there any reason to EVER call DoEvents?

No, a service does not need this, it shouldn't even need have a reference to System.Windows.Forms where the Application class is defined.

 

[kdbarrett]

That's what I thought. Thanks for your feedback!