views:

128

answers:

9

I don't like wide code, especially when it forces me to scroll. So having written this:

If _item.SubItems(pd.perioddate).Text = "N/A" Or _item.SubItems(pd.perioddate).Text = String.Empty Then
            dtpDeadlineforAP.Checked = False
End If

Is there a decent way to thin it down, make it more elegant?

+5  A: 

Extract _item.SubItems(pd.perioddate).Text into a local variable, e.g.

String text = _item.SubItems(pd.perioddate).Text

If text = "N/A" Or text = String.Empty Then
            dtpDeadlineforAP.Checked = False
End If

Alternatively, you may want to extract the whole check into a separate method:

If isNotFilled(_item.SubItems(pd.perioddate)) Then
            dtpDeadlineforAP.Checked = False
End If

This would make the code more readable and allow you to reuse the checking logic.

Péter Török
I would rename text variable to mean something, dim periodDate = _item.SubItems(pd.perioddate).Text
Iain
We should also consider Select Case rather than introducing a local variable? `Select Case _item.SubItems(pd.perioddate).Text Case "N/A", "" dtpDeadlineforAP.Checked = False End Select`
MarkJ
@Iain, agreed, I just could not find a good name - was considering `findSomeMeaningfulNameForThis` for a minute :-)
Péter Török
+1  A: 
string obj = _item.SubItems(pd.perioddate).Text;

If obj = "N/A" Or obj = String.Empty Then
            dtpDeadlineforAP.Checked = False
End If

ALSO

enable Word Warp in visual studio to stop having to scroll.

go to

Tools->Options->Text Editor->All languages->Word Warp

dont forget to enable 'Show all settings'

Stefanvds
@Stefanvds: The OP's question is in Visual Basic - the first line of your answer is in C#.
Jazza
@Jazza: I think VB users are able to (and used to) ignore a `;`
Henk Holterman
@Jazza, @Henk Yes, and we can also mentally change `string obj` into `Dim obj As String` :)
MarkJ
A: 
Dim date as String = _item.SubItems(pd.perioddate).Text

If date = "N/A" Or date = String.Empty Then 
            dtpDeadlineforAP.Checked = False 
End If
InSane
+1  A: 
With _item.SubItems(pd.perioddate)
    If .Text = "N/A" Or .Text = String.Empty Then
        dtpDeadlineforAP.Checked = False
    End If
End With

Cue argument regarding the merits/evils of WITH :)

El Ronnoco
+1  A: 

If there are multiple fields that can contain N/A, I'd suggest the following approach:

Dim invalidValues As String() = {"N/A", String.Empty}

If invalidValues.Contains(_item.SubItems(pd.perioddate).Text) Then
    dtpDeadlineforAP.Checked = False
End If

Or, if it's just about scrolling, you can use the VB line continuation character _:

If _item.SubItems(pd.perioddate).Text = "N/A" _
Or _item.SubItems(pd.perioddate).Text = String.Empty Then
    dtpDeadlineforAP.Checked = False
End If

BTW: Here, I'd suggest OrElse instead of Or.

Heinzi
A: 

As others suggested, you can use a local variable. You could also shorten the line additionally, by using the line continuation character _.

String period = _item.SubItems(pd.perioddate);

If period = "N/A" Or _
   period = String.Empty Then
        dtpDeadlineforAP.Checked = False
End If
Franci Penov
A: 

I would suppgest an Helper Class or ExtensionMethod:

If StringIsNullOrEmptyOrNA(stringval) Then
     ...
End If


If stringval.IsNullOrEmptyOrNa() Then
     ....
End If


Public Function StringIsNullOrEmptyOrNA(ByVal input as String) as Boolean
    return String.IsNullOrEmpty(input) OrElse input.Equals("N/A")
End Function


<System.Runtime.CompilerServices.Extension()> 
Public Function IsNullOrEmptyOrNa(ByVal input As String)
    return String.IsNullOrEmpty(input) OrElse input.Equals("N/A")
End Sub
SchlaWiener
@SchlaWiener: bear in mind that extension methods were introduced with VB.Net version 9.0 (VS 2008). The OP does not mention which version of VB.Net/VS he is using.
Jazza
That's why I added examples for both ways in the first place ;)
SchlaWiener
+1  A: 

We should at least mention Select Case

Select Case _item.SubItems(pd.perioddate).Text     
  Case "N/A", "" 
    dtpDeadlineforAP.Checked = False 
End Select 

Also consider extracting a helper function

Function IsNotApplicable(ByVal s As String) As Boolean
  Return (s = "N/A") Or (s = "")
End Function
MarkJ
A: 

I got it down to 3 lines and 63 columns. I have replaced the traditional If construct with the newer If operator. The code will also handle the case of Text being a null reference and will short-circuit using the OrElse operator. If you are willing to declare a couple of extension methods you could whittle this down to one short line, but that conflicts with the spirit of my answer.

Dim tx = _item.SubItems(pd.perioddate).Text
Dim dtp = dtpDeadlineforAP
dtp.Checked = If(tx = "N/A" OrElse tx = "", False, dtp.Checked)
Brian Gideon
I think we can still improve. VB.Net treats a null reference string as equal to "", so the whole first line could be dropped.
MarkJ
@MarkJ: I was not aware of that...until now. Thanks for pointing that out!
Brian Gideon