A coding style question

[deltaquattro]

Hello,

taking the cue from the previous thread I opened ("Error handling in
VBA"), I would like to ask you if in your opinion it's usually better
to check input data to a subroutine, inside the subroutine, or to move
each check to a distinct subroutine. As an example, I'll post again my
Interp subroutine, this time with some modifications added by Peter T:

Public Function Interp2(xArr() As Double, yArr() As Double, _
x As Double) As Double
Dim i As Long

' this is the part ...

If ((x < xArr(LBound(xArr))) Or (x > xArr(UBound(xArr)))) Then
'MsgBox "Interp2: x is out of bound" & "X = " & x

'.... I am considering moving outside the subroutine

Else

For i = LBound(xArr) To UBound(xArr)
If xArr(i) = x Then
Interp2 = yArr(i)
Exit For
ElseIf xArr(i) > x Then
Interp2 = yArr(i - 1) + (x - xArr(i - 1)) / _
(xArr(i) - xArr(i - 1)) * (yArr(i) - yArr(i
- 1))
Exit For
End If
Next i
End If
End Function

From one point of view, I would say yes, because a single subroutine
should just do a single thing, as someone on this ng recently
reminded me (don't remember who). On the other hand, if one follows
this principle too strictly, wouldn't he end writing "unsafe" code? If
I take out the check from Interp2 and move it to another subroutine
which the caller has to call before Interp2, I risk forgetting to add
the call to the checker subroutine when I reuse Interp2 in another
code. Sure, I could place the call to the checking sub inside Interp2:
however, this way I end up having a code with the same functionalities
as before, but with the added slowdown due to the call to the checking
subroutine.

Best Regards

Sergio

 

[Rick Rothstein]

If you mean the test in your If..Then statement... no, I would *not* move it
to a separate subroutine. Personally, I think the the principal of one
action per subroutine kind of overdoes it sometimes, but your question would
not really fall under this principal as I understand it.

 

[Chip Pearson]

Broadly speaking, the validation of input data to a procedure should
be within the procedure itself, and should execute before any other
code. However, it might be the case that several different procedures
have the same validation needs. For example, you may need to validate
whether a server share is available, and do this validation in a
number of independent procedures. In this case, you would probably
want to move the actual data validation to its own proc and call that
proc whenever the validation is required. If multiple procedures need
the same validation and that validation is complex, then you should
put it in its own proc that returns a Boolean indicating whether the
data is valid.

There is no benefit, and there is some (very small) cost, to moving
data validation code out of the proc that needs it to another proc.
Taking it out to another proc just for the sake of taking it out buys
you nothing.

Whoever told you that a single proc should do a single thing was
correct and you would do well to follow that recommendation. However,
in this context, I would consider any input validation to be a
subordinate task of the primary task carried out by the proc.

Cordially,
Chip Pearson
Microsoft MVP 1998 - 2010
Pearson Software Consulting, LLC
www.cpearson.com
[email on web site]

 

[deltaquattro]

Hi Chip!

Thanks for the reply, and compliments for your website, it's very good
and full of interesting information (actually, it was the first site I
used to start learning VBA for Excel).
If I understood you correctly, you're saying that input data checking
is a subtask of the sub itself, so I would do well to leave the check
in Interp2. If the check was more complex, and used by many
subroutines, I should consider moving it to a separate sub CheckData,
and have the various subroutines such as Interp2 call CheckData, so
I'm sure that each time Interp2 is called, CheckData is called too.
Very well, I'll keep that suggestion in mind. Thank you for your help,

Best Regards

Andrea

Broadly speaking, the validation of input data to a procedure should
be within the procedure itself, and should execute before any other
code. However, it might be the case that several different procedures
have the same validation needs. For example, you may need to validate
whether a server share is available, and do this validation in a
number of independent procedures. In this case, you would probably
want to move the actual data validation to its own proc and call that
proc whenever the validation is required.  If multiple procedures need
the same validation and that validation is complex, then you should
put it in its own proc that returns a Boolean indicating whether the
data is valid.

There is no benefit, and there is some (very small) cost, to moving
data validation code out of the proc that needs it to another proc.
Taking it out to another proc just for the sake of taking it out buys
you nothing.

Whoever told you that a single proc should do a single thing was
correct and you would do well to follow that recommendation. However,
in this context, I would consider any input validation to be a
subordinate task of the primary task carried out by the proc.

Cordially,
Chip Pearson
Microsoft MVP 1998 - 2010
Pearson Software Consulting, LLCwww.cpearson.com
[email on web site]

Hello,

taking the cue from the previous thread I opened ("Error handling in
VBA"), I would like to ask you if in your opinion it's usually better
to check input data to a subroutine, inside the subroutine, or to move
each check to a distinct subroutine. As an example, I'll post again my
Interp subroutine, this time with some modifications added by Peter T:

Public Function Interp2(xArr() As Double, yArr() As Double, _
                           x As Double) As Double
Dim i As Long

' this is the part ...

   If ((x < xArr(LBound(xArr))) Or (x > xArr(UBound(xArr)))) Then
       'MsgBox "Interp2: x is out of bound" & "X = " & x

'.... I am considering moving outside the subroutine

   Else

       For i = LBound(xArr) To UBound(xArr)
           If xArr(i) = x Then
               Interp2 = yArr(i)
               Exit For
           ElseIf xArr(i) > x Then
               Interp2 = yArr(i - 1) + (x - xArr(i - 1)) / _
                         (xArr(i) - xArr(i - 1)) * (yArr(i) - yArr(i
- 1))
               Exit For
           End If
       Next i
   End If
End Function

From one point of view, I would say yes, because a single subroutine
should just do a single thing, as someone on this ng  recently
reminded me (don't remember who). On the other hand, if one follows
this principle too strictly, wouldn't he end writing "unsafe" code? If
I take out the check from Interp2 and move it to another subroutine
which the caller has to call before Interp2,  I risk forgetting to add
the call to the checker subroutine when I reuse Interp2 in another
code. Sure, I could place the call to the checking sub inside Interp2:
however, this way I end up having a code with the same functionalities
as before, but with the added slowdown due to the call to the checking
subroutine.

Best Regards

Sergio

 

[deltaquattro]

Hi, Rick,

thanks for the reply, I'll do this way, as also suggested by Chip.

Best Regards

 

[Chip Pearson]

It is a generally accept princiiple of good programming never to do
the same thing in two different places (except for the most trival
things). If you have a block of code in two or more locations, then
you should consider taking that code, putting it in its own procedure,
and the call that procedure when necessary.

In your example, this would imply that you should leave the data
validation code in Interp2, where it is immediately needed. However,
if you find that as you write and expand the application, you are
using the same code many times in many different procs, then it is
time to think about moving the validation code to its own procedure.
There are no hard and fast rules for making that decision. Generally,
it boils down to personal coding style.

Minimizing duplicate code has many advantages (and very few, if any,
real disadvantages). First and foremost is that you need to write and
test the code once, and once tested, it can be called from any
procedure by just calling the function, no rewrites necessary. It also
makes debugging, ehancement, and maintenance tasks much easier since
you (or the person who inherits the from you) only need to revise on
piece of code, rather than trying to hunt down all the occurrences and
their variations.

I tend to think of much of the code tasks to be similar to building
something out of Lego blocks. Over time, you accumulate a standard
library of modules and functions related to a task, say, for example,
working with the System Registry. I have a module that contains all
sorts of code for working with the registry. If I need registry
functionality in an application, I just import the modRegistry module,
and that's the end it. No additional coding or testing is required. I
have around 200 VBA module designed to carry out a large variety of
tasks, and the ability to just import a module that is self-contained
and tested can save many, many, hours during developement.

Cordially,
Chip Pearson
Microsoft MVP 1998 - 2010
Pearson Software Consulting, LLC
www.cpearson.com
[email on web site]

Hi Chip!

Thanks for the reply, and compliments for your website, it's very good
and full of interesting information (actually, it was the first site I
used to start learning VBA for Excel).
If I understood you correctly, you're saying that input data checking
is a subtask of the sub itself, so I would do well to leave the check
in Interp2. If the check was more complex, and used by many
subroutines, I should consider moving it to a separate sub CheckData,
and have the various subroutines such as Interp2 call CheckData, so
I'm sure that each time Interp2 is called, CheckData is called too.
Very well, I'll keep that suggestion in mind. Thank you for your help,

Best Regards

Andrea

Broadly speaking, the validation of input data to a procedure should
be within the procedure itself, and should execute before any other
code. However, it might be the case that several different procedures
have the same validation needs. For example, you may need to validate
whether a server share is available, and do this validation in a
number of independent procedures. In this case, you would probably
want to move the actual data validation to its own proc and call that
proc whenever the validation is required.  If multiple procedures need
the same validation and that validation is complex, then you should
put it in its own proc that returns a Boolean indicating whether the
data is valid.

There is no benefit, and there is some (very small) cost, to moving
data validation code out of the proc that needs it to another proc.
Taking it out to another proc just for the sake of taking it out buys
you nothing.

Whoever told you that a single proc should do a single thing was
correct and you would do well to follow that recommendation. However,
in this context, I would consider any input validation to be a
subordinate task of the primary task carried out by the proc.

Cordially,
Chip Pearson
Microsoft MVP 1998 - 2010
Pearson Software Consulting, LLCwww.cpearson.com
[email on web site]

Hello,

taking the cue from the previous thread I opened ("Error handling in
VBA"), I would like to ask you if in your opinion it's usually better
to check input data to a subroutine, inside the subroutine, or to move
each check to a distinct subroutine. As an example, I'll post again my
Interp subroutine, this time with some modifications added by Peter T:

Public Function Interp2(xArr() As Double, yArr() As Double, _
                           x As Double) As Double
Dim i As Long

' this is the part ...

   If ((x < xArr(LBound(xArr))) Or (x > xArr(UBound(xArr)))) Then
       'MsgBox "Interp2: x is out of bound" & "X = " & x

'.... I am considering moving outside the subroutine

   Else

       For i = LBound(xArr) To UBound(xArr)
           If xArr(i) = x Then
               Interp2 = yArr(i)
               Exit For
           ElseIf xArr(i) > x Then
               Interp2 = yArr(i - 1) + (x - xArr(i - 1)) / _
                         (xArr(i) - xArr(i - 1)) * (yArr(i) - yArr(i
- 1))
               Exit For
           End If
       Next i
   End If
End Function

From one point of view, I would say yes, because a single subroutine
should just do a single thing, as someone on this ng  recently
reminded me (don't remember who). On the other hand, if one follows
this principle too strictly, wouldn't he end writing "unsafe" code? If
I take out the check from Interp2 and move it to another subroutine
which the caller has to call before Interp2,  I risk forgetting to add
the call to the checker subroutine when I reuse Interp2 in another
code. Sure, I could place the call to the checking sub inside Interp2:
however, this way I end up having a code with the same functionalities
as before, but with the added slowdown due to the call to the checking
subroutine.

Best Regards

Sergio