Coding style

  • Thread starter Thread starter Guest
  • Start date Start date
Cor Ligthert said:
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.
Click to expand...

Sorry, but the sample I posted is shorter than yours and less complex. It
depicts the way I am thinking, so I think it's perfect.
A computer is not human. Setting a bit is less work than testing a bit.
Click to expand...

I am wondering where you are testing fewer bits than I do in my sample. The
only difference I am able to see is that you are making an unnecessary
assignment.
 
Jonathan Wood said:
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?"
Click to expand...

ACK, especially with today's IDEs and syntax highlighting it should be easy
to distinguish between comparisons and assignments. Personally I do not
even need syntax highlighting to be able to determine if a '=' is an
assignment or a comparison operator in VB.NET.
 
Well, I do know some big companies that require programmers code in that
style. As mentioned elsewhere in this thread, the main reason seems to be
with C, where you could type = instead of == and, with a constant on the
left, the compiler would report an error.

But even that's very iffy (I don't code C that way), and there certainly
doesn't appear to be much reason for it with VB.

In the end, it's a matter of tast. But you don't just tell someone they
should be doing it that way without explanation as though that's the only
way to do it.
 
Branco

That is true and in that case you can use a dummy to set it. By the way I
showed this to show how you can do complex "if". I would use in this case
actual the way as is showed by Herfried, in that everybody can in one view
see what is happening.

Cor
 
Jonathan,

My method shows an alternative for where there are two long comparions
needed. In this actual case is the code as Herfried shows is in my opinion
the best

Cor
 
Herfried,

Did I in any way deny that, but that what you had, as I told you 4 hours
before your message, I had your code some minutes earlier than you showed
it.

I did not show it because it did not represent what I wanted to show:
Statements should be understandable in one view when reviewing code (not in
this particular case because the code you showed fullfills that).

In this case I would use the code as you have showed myself too.
But if it comes more complex I would not do that.

(Mixing up by instance logical and boolean "Or" or whatever in one
sentence).

Cor
 
Jonathan Wood said:
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
Click to expand...

If I understand the question correctly then this code is incorrect.
The original code was to set the enabled property to true if certain
conditions were met.
It tested for 3 conditions. The fourth possible condition was the default
current state.
ie. we can infer that the enabled property prior to the test was set to
false.
In the code above, the following situation will incorrectly set the enabled
value to true:
If Me.CheckBoxPrimaryYN.Checked = True And dr.PrimarySet = 0

Why? The original code tested 3 situations with the fourth being the
default.
The code above tests all four situations rather than only testing the three
required.
This is a comon mistake I find when programmers try to 'shortcut' another
programmer's code.

Cheers,
Greg
 
Sergey 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...

Is it me or aren't statements 1 and 2 not equal.
statement 1 states that the checkbox is enabled when:
checked=true and primaryset>0
or
checked=false and primaryset=0

Statement 2 states that the checkbox is enabled when:
either of the 2 statements is false so:
either checked=true or primaryset<>1
splitting up:

when checked=true, primaryset doesn't matter ' (false and something =
false) this contradicts statement 1 where if checked=true primaryset
must be > 0

when checked=false primaryset must be unequal to 1 ' without knowing
the valuerange of primaryset, this also contradicts statement 1 where
primary set must be 0. x=0 and x<>1 are only the same thing if the
valuerange of x is 0-1.
 
Greg,
If I understand the question correctly then this code is incorrect.
The original code was to set the enabled property to true if certain
conditions were met.
It tested for 3 conditions. The fourth possible condition was the default
current state.
ie. we can infer that the enabled property prior to the test was set to
false.
In the code above, the following situation will incorrectly set the
enabled value to true:
If Me.CheckBoxPrimaryYN.Checked = True And dr.PrimarySet = 0
Why? The original code tested 3 situations with the fourth being the
default.
The code above tests all four situations rather than only testing the
three required.
This is a comon mistake I find when programmers try to 'shortcut' another
programmer's code.
Click to expand...

Did you see the OP's compact code?

"Me.CheckBoxPrimaryYN.Enabled = Not (Me.CheckBoxPrimaryYN.Checked = False
And dr.PrimarySet = 1)"

The code I presented duplicates this logic exactly.

Yes, there were a few problems with the initial post, and of them was that
the compact version has slightly different logic than the longer version.
But I made the best of what I had to work with. And I see no errors in what
I presented.
 
Your code was less efficient even though you contended it was more
efficient. Then again, none of your replies seemed to directly respond to
the post you replied to so this entire exchange was rather tedious.
 
Jonathan Wood said:
Your code was less efficient even though you contended it was more
efficient. Then again, none of your replies seemed to directly respond to
the post you replied to so this entire exchange was rather tedious.
Click to expand...

Honestly, does it really matter? In cases where bit-twiddling really
matters, sure, it would matter...

The following is probably the most efficient code...but hey, does it matter?

' --------------------------------------------------------------
Dim checked As Boolean = Me.CheckBoxPrimaryYN.Checked
Dim primarySet As Integer = dr.PrimarySet

If primarySet >= 0
CheckBoxPrimaryYN.Enabled = _
(checked AndAlso primarySet > 0) _
OrElse (Not checked AndAlso primarySet = 0)
End If
' --------------------------------------------------------------

Anywho, my 2 cents.

Mythran
 
Jonathan Wood said:
Greg,


Did you see the OP's compact code?

"Me.CheckBoxPrimaryYN.Enabled = Not (Me.CheckBoxPrimaryYN.Checked = False
And dr.PrimarySet = 1)"

The code I presented duplicates this logic exactly.

Yes, there were a few problems with the initial post, and of them was that
the compact version has slightly different logic than the longer version.
But I made the best of what I had to work with. And I see no errors in
what I presented.
Click to expand...

Jonathan,

Your code is certainly correct as an alternative to the OP's second solution
where all four possiblities are tested.
The problem lies with the OP's first solution which only tests for three of
the four possibilities suggesting the missing possibility is the orignigal
state of the CheckBox's Enable property. I only compared your solution to
the OP's first solution (I didn't look at the OP's second solution as I
assumed his team were arguing about the best/easiest way to write the *same*
test). Apologies for the confusion. Keep up the good work.

Regards,
Greg
 
C-Services Holland b.v. said:
Is it me or aren't statements 1 and 2 not equal.
statement 1 states that the checkbox is enabled when:
checked=true and primaryset>0
or
checked=false and primaryset=0

Statement 2 states that the checkbox is enabled when:
either of the 2 statements is false so:
either checked=true or primaryset<>1
splitting up:

when checked=true, primaryset doesn't matter ' (false and something =
false) this contradicts statement 1 where if checked=true primaryset must
be > 0

when checked=false primaryset must be unequal to 1 ' without knowing the
valuerange of primaryset, this also contradicts statement 1 where primary
set must be 0. x=0 and x<>1 are only the same thing if the valuerange of x
is 0-1.
Click to expand...

No it's not you. Statements 1 and 2 are *definitely* different.
Statement 2 tests all four different possibilities whereas Statement 1 only
tests for three of the four possibilities suggesting the missing possibility
is the orignigal state of the CheckBox's Enable property.

Cheers,
Greg.
 
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...

G'day. Statement1 and Statement2 do not do the same thing. Statement2 is a
nice, easy way to test all four possible conditions. However, if you only
want to test three of the four conditions, then you'd need to use
Statement1. There are many ways to re-code your two statements. Here's a
couple I'd use:

'Statement 1 - Testing 3 of 4 possibilities
If Not (Me.CheckBoxPrimaryYN.Checked = True And dr.PrimarySet = 0) Then
If Me.CheckBoxPrimaryYN.Checked = False And dr.PrimarySet = 1 Then
Me.CheckBoxPrimaryYN.Enabled = False
Else
Me.CheckBoxPrimaryYN.Enabled = True
End If
End If

'Statement2 - Testing all 4 possibilities
If Me.CheckBoxPrimaryYN.Checked = False And dr.PrimarySet = 1 Then
Me.CheckBoxPrimaryYN.Enabled = False
Else
Me.CheckBoxPrimaryYN.Enabled = True
End If

Cheers,
Greg
 
Sergey 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...

1 is clearer, but harder to read and see where it "fits in". 2 is more
succinct, but not as clear.

Normally, i'd add a line or so for clarity, as it is much easier to
read the code later. However, this seems a bit too bulky. I'd go with
the second add but a decent comment. If there are no comments, 1 is
better.

B.
 
Mythran,
Honestly, does it really matter? In cases where bit-twiddling really
matters, sure, it would matter...
Click to expand...

It must matter as the person I was replying to kept posting that his code
was faster, and now you've posted the same thing.
The following is probably the most efficient code...but hey, does it
matter?
Click to expand...
Dim checked As Boolean = Me.CheckBoxPrimaryYN.Checked
Dim primarySet As Integer = dr.PrimarySet

If primarySet >= 0
CheckBoxPrimaryYN.Enabled = _
(checked AndAlso primarySet > 0) _
OrElse (Not checked AndAlso primarySet = 0)
End If
Click to expand...

Since my code only read CheckBoxPrimaryYN.Checked once, I would say it is
more efficient than reading it once, copying it to a variable and then
reading that variable twice. And since my code only checks dr.PrimarySet
once, I would say it is more efficient than reading it once, copying it to a
variable and then test that variable three times.

Looking at my code again, it appears it could be further optimized. Note
that I am using the logic of the second version provided by the OP. This is
the syntax I would recommend (I believe it is correct):

If dr.PrimarySet <> 1 Then
Me.CheckBoxPrimaryYN.Checked = True
End If
 
Jonathan Wood said:
Since my code only read CheckBoxPrimaryYN.Checked once, I would say it is
more efficient than reading it once, copying it to a variable and then
reading that variable twice. And since my code only checks dr.PrimarySet
once, I would say it is more efficient than reading it once, copying it to
a variable and then test that variable three times.

Looking at my code again, it appears it could be further optimized. Note
that I am using the logic of the second version provided by the OP. This
is the syntax I would recommend (I believe it is correct):

If dr.PrimarySet <> 1 Then
Me.CheckBoxPrimaryYN.Checked = True
End If
Click to expand...

Hi Jonathon,
Your latest code doesn't cater for two of the four tests provided by the
OP's code in Statement2 ?

After some thought, I'd rewrite the OP's Statement2 as follows to cover all
four scenarios:

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

Cheers,
Greg
 
Greg,
Hi Jonathon,
Your latest code doesn't cater for two of the four tests provided by the
OP's code in Statement2 ?
Click to expand...

Please provide a scenario (state of variables tested) where the code above
produces different results than the OP's statement 2.

Thanks.
 
Jonathan Wood said:
Greg,


Please provide a scenario (state of variables tested) where the code above
produces different results than the OP's statement 2.

Thanks.
Click to expand...

Actually looking at your code, you're not setting the CheckBox's Enabled
property at all which is the whole point of the OP's Statement2 code?

Cheers.
 
Greg,
Actually looking at your code, you're not setting the CheckBox's Enabled
property at all which is the whole point of the OP's Statement2 code?
Click to expand...

So far, you seem to just be assuming I'm pretty darn stupid.

So I ask again: Please provide a scenario (state of variables tested) where
my code produces a different results than the OP's statement 2.

I'd be more than happy to discuss this further with you. I may just be
smarter than you think I am. But if you're just going to respond again that
it's wrong, then don't bother.
 
Back
Top