Coding style

  • Thread starter Thread starter Guest
  • Start date Start date
Okay, I stand corrected. Somewhere along the line, I was mistakenly thinking
that it was the Checked property that was being set.

Therefore, I'd stand by my original code:

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

Thanks.
 
Jonathan Wood said:
Okay, I stand corrected. Somewhere along the line, I was mistakenly
thinking that it was the Checked property that was being set.

Therefore, I'd stand by my original code:

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

Thanks.
Click to expand...

No problems.
Good, eay to read code.
However, as a personal choice, I'd convert it to a single line IIf statement
to use less lines:

Me.CheckBoxPrimaryYN.Enabled = IIf(Not Me.CheckBoxPrimaryYN.Checked And
Me.dr.PrimarySet, False, True)

Cheers.
 
Greg said:
No problems.
Good, eay to read code.
However, as a personal choice, I'd convert it to a single line IIf
statement to use less lines:

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

And, obviously, I'd lose the unneccessary "Me."s giving:

CheckBoxPrimaryYN.Enabled = IIf(Not CheckBoxPrimaryYN.Checked And
dr.PrimarySet, False, True)

Cheers.
 
Jonathan Wood said:
Okay, I stand corrected. Somewhere along the line, I was mistakenly
thinking that it was the Checked property that was being set.

Therefore, I'd stand by my original code:

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

.... where I'd use:

\\\
Me.CheckBoxPrimaryYN.Enabled = _
Me.CheckBoxPrimaryYN.Checked AndAlso dr.PrimarySet <> 1
///
 
Greg said:
CheckBoxPrimaryYN.Enabled = IIf(Not CheckBoxPrimaryYN.Checked And
dr.PrimarySet, False, True)
Click to expand...

I'd loose the unnecessary 'IIf' with its additional boxing overhead too,
because 'Not CheckBoxPrimaryNY.Checked AndAlso dr.PrimarySet = 1' already
returns a boolean value.
 
Herfried,

I was thinking to do the answer the same as you,
I don't know why I decided not to do that.
(Maybe that I did not want to show again how I hate the IIF)

I get the idea that beside me nobody noticed your fine solution.

Cor
 
Okay, I stand corrected. Somewhere along the line, I was mistakenly
thinking that it was the Checked property that was being set.

Therefore, I'd stand by my original code:

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

I still prefer the single line version. Since I'm currently reading Code
Complete, I thought I would throw out another option just to stir the pot
how about refactoring the logic into a separate sub and doing the following.
It may be slightly more overhead, but may aid in maintainability.

CheckBoxPrimaryYN.Enabled = CanEnableBox()

Private Function CanEnableBox() as Boolean
Return Not CheckBoxPrimaryNY.Checked AndAlso dr.PrimarySet = 1
End Sub

Let's see if we can stir this discussion some more.

Jim Wooley
http://devauthority.com/blogs/jwoole
 
Yes, that's pretty efficient. However, other than using up less space being
slightly less readable, I don't think it's faster than my version.

In my old age, I've grown to prefer the most readable code over saving lines
of source code.

One item that could be more efficient though is use of AndAlso. I would
probably change my code to use that instead.
 
I'm not sure I see the benefits of moving it to its own function. And
definitely less efficient.
 
Based on my preferences, I generally avoid IIf. When I started programming,
I used to like one-liners like that. But these days, I have no problem
stretching out the source to make it easier to read and debug. IIf provides
no advantage over a regular If statement other than compacting your source
and making it slightly less readable. In addition, if IIf still involves an
additional call, then it's less efficient as well.

In addition to that, it is not necessary in your code as you could just do
this:

Me.CheckBoxPrimaryYN.Enabled = Not (Not Me.CheckBoxPrimaryYN.Checked And
Me.dr.PrimarySet)
 
Herfried K. Wagner said:
I'd loose the unnecessary 'IIf' with its additional boxing overhead too,
because 'Not CheckBoxPrimaryNY.Checked AndAlso dr.PrimarySet = 1' already
returns a boolean value.
Click to expand...

Thanks Herfried.

However, losing the IIf and using the AndAlso would then require an extra
Not. ie:

CheckBoxPrimaryYN.Enabled = Not (Not CheckBoxPrimaryYN.Checked AndAlso
dr.PrimarySet)

As the OP's question related to which statement is easier to read/maintain,
I'd prefer not to use double negatives.
If I was to pick up someone else's code and saw the code above, I'd have to
use a few brain cells to determine what it was trying to achieve. If,
however, I saw the code that Jonathan provided in an earlier post then I'd
have no trouble understanding what was happening.

So my answer to the OP is that I wouldn't use either of his Statement1 or
Statement2 suggestions. Instead, I'd use Jonathan's suggestion of:

If Not CheckBoxPrimaryYN.Checked And dr.PrimarySet Then
CheckBoxPrimaryYN.Enabled = False
Else
CheckBoxPrimaryYN.Enabled = True
End If

However, as the result of the If statement is only returning a single
action, I'd use the IIf statement.
If multiple actions were required as a result of the If statement, I'd
certainly choose Jonathan's format.

Cheers,
Greg.
 
Hi,

There are some discrepancies in the OP's examples. The conditions
stated in his first one aren't the same as those in the second.

According to the first example, we have the following conditions to
enable the checkbox:

a) It's checked and dr.PrimarySet > 0
b) It's unchecked and dr.PrimarySet = 0

And we have this condition to disable the CheckBox:

c) It's unchecked and dr.PrimarySet <> 0

Notice that nothing is said about what to do if the checkbox is checked
but dr.PrimarySet <= 0. According to the code, we do nothing, that is,
we leave the checkbox.Enabled property in it's current state. Now, it
may be part of the logic of the application or an omission. We have no
way to tell, since we're not informed about the possible values of
dr.PrimarySet.

Consider now the second example. According to it, we Disable the
Checkbox if:

a) It's not Checked and dr.Primary = 1;

otherwise we enable it.

This states a completelly different logic from the 1st example, and
there's no way to compare both, even though they deal with the same
objects. We can only say that the second example has no relation to the
first, or one of them is a completely mistaken representation.

The OP says he prefers the second example, and issues a statement
similar to:

CheckBox.Enabled = Not( Not CheckBox.Checked _
AndAlso dr.PrimarySet = 1)

To simplify this expression eliminating the double negation, we may
take advantage of the fact that:

Not (A And B) -> Not(A) Or Not(B)

Using our current expression (where A => CheckBox.Checked, B=>
dr.PrimarySet = 1):

Not(Not(A) And B)
-> Not(Not(A)) Or Not(B)
-> A Or Not B

Thus:

CheckBox.Enabled = CheckBox.Checked OrElse dr.PrimarySet <> 1


Now, if we were to simplify the logic of the first example, we should
decide what to do if the checkbox is checked and dr.PrimarySet <= 0.
I'll assume that when the OP stated dr.PrimarySet > 0, he actually
wanted to say dr.PrimarySet <> 0. I will also assume that if the
Checkbox is set but dr.PrimarySet = 0, we must not mess with the
Enabled property. In other words, to set the Enabled property we'll
have to consider it's current value!

These are, thus, our options:

A) CheckBox.Checked And dr.PrimarySet <> 0 -> Enabled = True
B) CheckBox.Checked And dr.PrimarySet = 0 -> Enabled = Enabled
C) CheckBox.Unchecked and dr.PrimarySet = 0 -> Enabled = True
D) CheckBox.Unchecked And dr.PrimarySet <> 0 -> Enabled = False

If we were to state this logic in a single expression, it could be:

Dim A As Boolean = CheckBox.Checked
Dim B As Boolean = dr.PrimarySet <> 0
Dim C As Boolean = CheckBox.Enabled

CheckBox.Enabled = Not(A OrElse B) _
OrElse (A AndAlso (B OrElse C))

Arguably, it's much more clear to use an If:

If CheckBox.Checked Then
CheckBox.Enabled = (dr.PrimarySet <> 0) OrElse CheckBox.Enabled
Else
CheckBox.Enabled = dr.PrimarySet = 0
End If

In any way, let's hope the VB designers introduce a ternary operator in
the language for the next version:

CheckBox.Enabled = CheckBox.Checked? _
(dr.PrimarySet <> 0) OrElse CheckBox.Enabled | dr.PrimarySet = 0

=))))

Have fun,

Branco
 
Greg said:
However, losing the IIf and using the AndAlso would then require an extra
Not. ie:

CheckBoxPrimaryYN.Enabled = Not (Not CheckBoxPrimaryYN.Checked AndAlso
dr.PrimarySet)

As the OP's question related to which statement is easier to
read/maintain, I'd prefer not to use double negatives.
Click to expand...

You could easily get rid of the double negation by applying De Morgan's law:

<URL:http://en.wikipedia.org/wiki/De_Morgan's_laws>:

NOT(A AND B) = (NOT A) OR (NOT B)
NOT(A OR B) = (NOT A) AND (NOT B)

=>

\\\
Me.CheckBoxPrimaryNY.Enabled = _
CheckBoxPrimaryYN.Checked OrElse Not dr.PrimarySet
 
Back
Top