Coding style

  • Thread starter Thread starter Guest
  • Start date Start date
G

Guest

Hello All
I work at software company and we are having a really big discussion about
coding styles and it seems that more people
prefer statement 1 to statement2 , which was a big surprise to me. Please
help us with your comments. Thanks

Which way is better way 1 or 2?

1. 'set enable status on CheckboxPrimaryYN

If (Me.CheckBoxPrimaryYN.Checked = True) And (dr.PrimarySet > 0) Then

Me.CheckBoxPrimaryYN.Enabled = True

Else

If (Me.CheckBoxPrimaryYN.Checked = False) Then

If (dr.PrimarySet = 0) Then

Me.CheckBoxPrimaryYN.Enabled = True

Else

Me.CheckBoxPrimaryYN.Enabled = False

End If

End If

End If



2. 'set enable status on CheckboxPrimaryYN

Me.CheckBoxPrimaryYN.Enabled = Not (Me.CheckBoxPrimaryYN.Checked =
False And dr.PrimarySet = 1)
 
Sergey Zuyev said:
Hello All
I work at software company and we are having a really big discussion
about
coding styles and it seems that more people
prefer statement 1 to statement2 , which was a big surprise to me. Please
help us with your comments. Thanks

Which way is better way 1 or 2?

1. 'set enable status on CheckboxPrimaryYN

If (Me.CheckBoxPrimaryYN.Checked = True) And (dr.PrimarySet > 0)
Then

Me.CheckBoxPrimaryYN.Enabled = True

Else

If (Me.CheckBoxPrimaryYN.Checked = False) Then

If (dr.PrimarySet = 0) Then

Me.CheckBoxPrimaryYN.Enabled = True

Else

Me.CheckBoxPrimaryYN.Enabled = False

End If

End If

End If



2. 'set enable status on CheckboxPrimaryYN

Me.CheckBoxPrimaryYN.Enabled = Not (Me.CheckBoxPrimaryYN.Checked =
False And dr.PrimarySet = 1)
Click to expand...

And it doesn't seem fair that you used so much wording for #1 when #1 could
also have been written as follows:

If CheckBoxPrimaryYN.Checked AndAlso dr.PrimarySet > 0
CheckBoxPrimaryYN.Enabled = True
ElseIf Not CheckBoxPrimaryYN.Checked
If dr.PrimarySet = 0
CheckBoxPrimaryYN.Enabled = True
Else
CheckBoxPrimaryYN.Enabled = False
End If
End If

Or I would have even taken the mix of both to simplify:

#3
If CheckBoxPrimaryYN.Checked AndAlso dr.PrimarySet > 0
CheckBoxPrimaryYN.Enabled = True
ElseIf Not CheckBoxPrimaryYN.Checked
CheckBoxPrimaryYN.Enabled = dr.PrimarySet = 0
End If

The reason behind why so many people would prefer #1 over #2 is
simplification. Otherwords, it's easier to follow #1 than it is #2 (because
the if's are broken down and not compacted onto a single line). People can
follow the if's easier and get the big picture rather than having them
compressed onto a single line and have to use more brainpower to expand the
logic.


HTH :)

Mythran
 
I would prefer #2 with a twist:

Me.CheckBoxPrimaryYN.Enabled = Me.CheckBoxPrimaryYN.Checked or
dr.PrimarySet <> 1

or if you prefer the other condition for clarity, then:

Me.CheckBoxPrimaryYN.Enabled = Not (Not Me.CheckBoxPrimaryYN.Checked And
dr.PrimarySet = 1)
 
I've been programming many years now and, in general, I tend to prefer the
longer expressions because they often improve readability with no cost to
performance. However, here, I actually think your longer expression is too
convoluted. In addition, it checks CheckBoxPrimaryYN.Checked property
several times, which could impact performance.

So the comparison would be more fair if your "longer version" looked
something like this:

If Me.CheckBoxPrimaryYN.Checked = False And dr.PrimarySet = 1)
Me.CheckBoxPrimaryYN.Enabled = False
Else
Me.CheckBoxPrimaryYN.Enabled = True
End If

That said, I think I personally would go ahead with your short version. I
find it easier to understand than your long version (although not
necessarily easier to understand than my long version). And it accesses each
variable/property only once, which means that its probably a little more
efficient.
 
I would typically prefer the condensed version #2 as well, potentially with
one of the logic extensions mentioned before. The big downside of the first
method (nested if's), other than maintainability is the possibility that
you won't reset the enabled value on one of the else blocks. In your sample
code, I believe the (Me.CheckBoxPrimaryYN.Checked = False) evaluation won't
reset the enabled status if it is not checked. This may be by design, but
I have seen the case often enough that it is simply an oversight when the
direct assignment of option 2 wouldn't have that problem which helps to facilitate
the maintainability and reliability of the code.

One additional thing to consider which may optomize the expression is to
use logical short-circuting (AndAlso/OrElse) rather than the simple logical
evaluation.

Jim Wooley
http://devauthority.com/blogs/jwooley/default.aspx
 
Sergey,

I find #2 an expression for a programmer who likes to obfuscate his/her
sources for others.

It has not even any performanse benefit, but probably took more time to
create and debug than straight written code.

Mostly it is the most simple to set in your code as in this sample the
checkbox to enabled to true, and than check if it maybe should be false and
set it like that, that cost as well the less performance.

Don't expect that the user sees it that you set it to enabled is true. Our
eyes are not fast enough for that while it is not even painted in the
method.

Just my opinion.

Cor
 
Hello Sergey,

#2 is definately preferable. I would make a few minor changes for clarity
though..

1. When testing for equality within an assignment, always place the constant
to the left of the equality test.
(example: If 1 = x Then ... since you can not assignthe value of x to
1 this is obviously an equality test, not an assignment.)

2. Always group your operation in parenthesees. Yes.. we all know order
of operation will take care of it.. but if you group your operations it's
easy to see exactly whats going on at a glance. Also grouping them makes
it clear where the boolean operations are happening.

So.. to re-write #2 using these rules:
Me.CheckBoxPrimaryYN.Enabled = Not ((Me.CheckBoxPrimaryYN.Checked = False)
And (1 = dr.PrimarySet))


Comments on your #1 snippet:

1. Always write your numeric tests like you are reading a number line.
(example: If you want to know if dr.PrimarySet is greater than 0 then
write:
If 0 < dr.PrimarySet Then ...)

-Boo
 
For those who don't understand what I mean.

\\\
Me.CheckBoxPrimaryYN.Enabled = True
If Me.CheckBoxPrimaryYN.Checked = False AndAlso dr.PrimarySet <> 0 then
me.CheckBoxPrimaryYN.Enabled = False
End if.
///

Cor
 
Sergey Zuyev said:
I work at software company and we are having a really big discussion
about
coding styles and it seems that more people
prefer statement 1 to statement2 , which was a big surprise to me.

Which way is better way 1 or 2?

1. 'set enable status on CheckboxPrimaryYN

If (Me.CheckBoxPrimaryYN.Checked = True) And (dr.PrimarySet > 0)
Then

Me.CheckBoxPrimaryYN.Enabled = True

Else

If (Me.CheckBoxPrimaryYN.Checked = False) Then

If (dr.PrimarySet = 0) Then

Me.CheckBoxPrimaryYN.Enabled = True

Else

Me.CheckBoxPrimaryYN.Enabled = False

End If

End If

End If



2. 'set enable status on CheckboxPrimaryYN

Me.CheckBoxPrimaryYN.Enabled = Not (Me.CheckBoxPrimaryYN.Checked =
False And dr.PrimarySet = 1)
Click to expand...

I'd use a simplified version of the the latter solution:

\\\
Me.CheckBoxPrimaryYN.Enabled = _
Not Me.CheckBoxPrimaryYN.Checked AndAlso dr.PrimarySet <> 1
///
 
Herfried,

I saw that one as well while writing my sample but did not want to go in
another direction.

:-)

Cor
 
Cor Ligthert said:
For those who don't understand what I mean.

\\\
Me.CheckBoxPrimaryYN.Enabled = True
If Me.CheckBoxPrimaryYN.Checked = False AndAlso dr.PrimarySet <> 0 then
me.CheckBoxPrimaryYN.Enabled = False
End if.
///
Click to expand...

I don't think this solution is very intuitive because the property is being
set to a certain value twice, overwriting the previous value, which
unnecessarily increases the complexity of the code.
 
Herfried,

Your answer is as most people think. They think that setting a bit is more
work than first testing it. That is as we humans do it, therefore are our
brains.

A computer is not human. Setting a bit is less work than testing a bit. I
know that it is really difficult to think like that for a human.

But see my reply on your sample.

:-)

Cor
 
Which way is better way 1 or 2?

I like 2 with comments added. Generally, I prefer shorter source constructs
over longer ones, but if the code is obscure or devious, I want comments that
describe intent.
 
GhostInAK,
1. When testing for equality within an assignment, always place the
constant to the left of the equality test.
(example: If 1 = x Then ... since you can not assignthe value of x to 1
this is obviously an equality test, not an assignment.)
Click to expand...

Ack. FWIW, I hate this approach. This style seems to have originated by
folks concerned about doing an assignment when a compare was intended. But
it's completely unintuitive. You don't ask "is red the color of the barn?"
You ask "Is the barn red?"

That's fine if you prefer this approach (and you're not working on any of my
projects <g>). But as general advice, given without any explanation, I feel
this item is out of place.

Just my opinion.
2. Always group your operation in parenthesees. Yes.. we all know order
of operation will take care of it.. but if you group your operations it's
easy to see exactly whats going on at a glance. Also grouping them makes
it clear where the boolean operations are happening.
Click to expand...

This is a matter of taste also.
Comments on your #1 snippet:

1. Always write your numeric tests like you are reading a number line.
(example: If you want to know if dr.PrimarySet is greater than 0 then
write:
If 0 < dr.PrimarySet Then ...)
Click to expand...

Ack, cough!
 
My version is slightly more efficient since yours sometimes requires the
property to be set twice.
 
Cor,
Your answer is as most people think. They think that setting a bit is more
work than first testing it. That is as we humans do it, therefore are our
brains.

A computer is not human. Setting a bit is less work than testing a bit. I
know that it is really difficult to think like that for a human.
Click to expand...

But your example does both. It is less efficient.
 
Cor Ligthert [MVP] wrote:
\\\
Me.CheckBoxPrimaryYN.Enabled = True
If Me.CheckBoxPrimaryYN.Checked = False AndAlso dr.PrimarySet <> 0 then
me.CheckBoxPrimaryYN.Enabled = False
End if.
///
Click to expand...
<snip>

Your code assumes that setting CheckBoxPrimaryYN.Enabled has no side
effects.

Actually, it probably involves much more than setting a bit: It
probably will, at least, invalidade the area where the checkbox lies
in.

Besides, if there are any actions associated with the event
CheckBoxPrimaryYN.EnableChanged, more side effects may also trigger,
eventually disrupting the application state.

Regards,

Branco.
 
Jonathan,

That is the human mind, who suposes that testing first is more efficient
while the testing takes the most time.

This is the most simple sample that you often see
If A <> 1 then A = 1.

This is much quicker and less code for the computer
A = 1.

Cor

Jonathan Wood said:
My version is slightly more efficient since yours sometimes requires the
property to be set twice.
Click to expand...
 
GhostInAK wrote:
1. When testing for equality within an assignment, always place the constant
to the left of the equality test.
(example: If 1 = x Then ... since you can not assignthe value of x to
1 this is obviously an equality test, not an assignment.)
Click to expand...
<snip>

The assignment operator in VB doesn't act like an expression, so
there's no chance you mistake an equality test with an assignment.

Your suggestion would be more sound if VB performed like the C language
(and similars). In C, the equality operator is '==', so

C == 0

is a comparison, while

C = 0

is an assignment.

More over, in C you can treat comparisons as statements and assignments
as expressions. Therefore,

C == 0;

is a valid statement in C (albeit a harmless one), and

if (C=0) break;

uses an (syntactically correct) assignment as an expression (it assigns
0 to C, tests the result -- which will be 0 -- and breaks if the result
is true -- which it isn't).

As it can be seen, in the C planet it's very easy for the programmer to
write an assignment when he/she meant a comparision, with absolutelly
no help from the compiler to detect the mistake. So the idiom of
placing the constant before the comparision operator is more like a
safeguard. A really advisable one.

This has *zero* chances of happening is VB, the semantics are quite
different. Therefore, this idiom is literally alien to VB, and will
bring no benefit whatsoever other than annoying a few code-readers
(yours trully included).

Regards,

Branco.
 
You don't appear to be following. Yes, the first one is slower because it
must perform BOTH a comparison and an assignment. But your sample does both.
You have potentially two comparisons and potentially two assignments. The
code I presented has potentially two comparisons but only one assignment.
That's less, and so it's more efficient. Why would you think your version is
more efficient?
 
Back
Top