1
\$\begingroup\$

Is it possible to shorten the Isnull checking?

It would be great to shorten this code and only have to put the datarow once.

This code imports data details (usually there would be more .x = IIF.... statements)

 Public Class apPropertyFunctions

        Public Function GetProperty(ByVal sUPRN As String) As apProperty
            'Public Function GetProperty(ByVal sUPRN As String, ByVal sUCARN As String) As apProperty
            Try

                Dim vIP As String = HttpContext.Current.Request.ServerVariables("REMOTE_ADDR")
                Dim oDetails As New apProperty

                Dim sConnString As String = System.Web.Configuration.WebConfigurationManager.ConnectionStrings("SQL_DEMO").ConnectionString
                Dim dsNames As SqlDataSource

                dsNames = New SqlDataSource

                dsNames.ConnectionString = sConnString
                Dim sSQL As String
                sSQL = "spGetProperty"
                dsNames.SelectCommand = sSQL
                dsNames.SelectCommandType = SqlDataSourceCommandType.StoredProcedure
                dsNames.SelectParameters.Clear()
                dsNames.SelectParameters.Add("ID", ID)

                For Each datarow As Data.DataRowView In dsNames.Select(DataSourceSelectArguments.Empty)

                    With oDetails
                        .ID= datarow("ID").ToString    
     .BuildingNo = IIf(datarow("BuildingNo").Equals(DBNull.Value), "", datarow("BuildingNo").ToString)
                    End With

                    '  End If
                Next

                dsNames = Nothing

                Return oDetails

            Catch ex As Exception
                Return Nothing
            End Try

        End Function
\$\endgroup\$
  • 2
    \$\begingroup\$ It helps if reviewers understand the purpose of your code. Can you please add some description/explanation of what this code accomplishes? \$\endgroup\$ – RubberDuck Jan 8 '15 at 15:54
  • \$\begingroup\$ why do you open the class but not close it in the code excerpt? And you should probably give us more of the .x = IIF.... statments \$\endgroup\$ – Malachi Jan 8 '15 at 17:38
2
\$\begingroup\$

To actually answer the question that was asked, instead of just picking on the code.

To shorten

.BuildingNo = IIf(datarow("BuildingNo").Equals(DBNull.Value), "", datarow("BuildingNo").ToString)

you could use

.BuildingNo = If(TryCast(datarow("BuildingNo"), String), "")

I just tried this in a test program and it worked. (VB.Net 2013)

And just to pick on a comment @RubberDuck made about Hungarian notation, I've been programming since it was just "BASIC", and suggestions about variable names tends to change every couple of years. I don't know how many programming books I have that suggested using Hungarian notation. No matter what you do, you're going to get picked on.

\$\endgroup\$
  • 1
    \$\begingroup\$ I too started back when it was just "BASIC". Back then it was a good idea, because variables weren't actually typed. In fact, I still use Hungarian notation when writing vbscript because everything can be anything in that language. That's not the case in .Net. .Net is strongly typed, thus there's no reason to use a notation system that causes extra maintenance /or code that lies. I backed up my recommendation while you've said "it's okay, I have old books that say so". \$\endgroup\$ – RubberDuck Jan 18 '15 at 12:29
  • \$\begingroup\$ I didn't/don't want to start a big argument about this, but I didn't say hungarian notation was OK, I just said that it used to be a standard, and no matter what you do, someone will find something to comment on. The OP was asking how to do something, and nobody was answering that question. I was just trying to make him feel not quite so picked on. But I'm sorry if you felt I implied that what you said was not the way to go. \$\endgroup\$ – OldTimer Jan 19 '15 at 18:30
  • \$\begingroup\$ I don't intend on arguing about Hungarian either. It just felt like you were saying "It's okay, that was once recommended." without having any real reasons it was ok. I'm happy to hear it clarified that you don't think it is either. As for answering the question asked, any and all aspects of the code are on topic for review. Answers don't necessarily need to address direct questions like that here. Although, I myself am a proponent of addressing them whenever possible. Sorry for any misunderstandings. \$\endgroup\$ – RubberDuck Jan 19 '15 at 19:28
5
\$\begingroup\$

Don't leave garbage laying around please, pick up after yourself.

'  End If

This doesn't have a purpose here, just get rid of it.


This isn't a good idea either

        Catch ex As Exception
            Return Nothing
        End Try

This was probably meant to close the connection, but there is a much better way to do that than to catch all exceptions and not report them. Using Statements.


This is kind of smelly to me as well.

            Dim sSQL As String
            sSQL = "spGetProperty"
            dsNames.SelectCommand = sSQL

This simple Function doesn't need to be any more difficult than it has to be, you should do one of two things here.

  1. Pass in the Stored Procedure Name as a string to the function
  2. just set the SelectCommand to the name of the stored procedure like:

    dsNames.SelectCommand = "spGetProperty"
    

It doesn't look like there is any reason whatsoever to call dsNames.SelectParameters.Clear() this object was created here and there is nothing in it yet, there should be no reason that you need to clear nothing.

Some other things that you don't need include

            Dim vIP As String = HttpContext.Current.Request.ServerVariables("REMOTE_ADDR")

And

        'Public Function GetProperty(ByVal sUPRN As String, ByVal sUCARN As String) As apProperty

With all that I left in the Try/Catch statement because I didn't know exactly what you were needing it for, but this is what it would look like with all these changes.

Public Function GetProperty(ByVal sUPRN As String) As apProperty
    Try
        Dim oDetails As New apProperty
        Dim sConnString As String = System.Web.Configuration.WebConfigurationManager.ConnectionStrings("SQL_DEMO").ConnectionString

        Using dsNames As New SqlDataSource
            dsNames.ConnectionString = sConnString
            dsNames.SelectCommand = "spGetProperty"
            dsNames.SelectCommandType = SqlDataSourceCommandType.StoredProcedure
            dsNames.SelectParameters.Clear()
            dsNames.SelectParameters.Add("ID", ID)

            For Each datarow As Data.DataRowView In dsNames.Select(DataSourceSelectArguments.Empty)
                With oDetails
                    .ID = datarow("ID").ToString    
                    .BuildingNo = IIf(datarow("BuildingNo").Equals(DBNull.Value), "", datarow("BuildingNo").ToString)
                End With
            Next
        End Using
        Return oDetails
    Catch ex As Exception
        Return Nothing
    End Try
End Function
\$\endgroup\$
4
\$\begingroup\$

For the specific question about optimizing the IIf statements, in general you should prefer the If operator, instead of the IIf function, since it uses short-circuit evaluation, which leads to less surprises.

The If operator also supports being called with two parameters, returning the first parameter if it is non-null, and the second parameter if the first was null. You can use this, in conjunction with the DataRowExtensions.Field(Of T) method, which will return Nothing instead of DBNull.Value for null values, to simplify the check:

With oDetails
    .ID = datarow.Field(Of String)("ID")
    .BuildingNo = If(datarow.Field(Of String)("BuildingNo"), String.Empty)
End With

Another option would be to create a function to handle the nulls and use that, e.g.

Public Function NCStr(input As Object) As String
    If IsDBNull(input) OrElse input Is Nothing Then Return String.Empty
    Return CStr(input)
End If
'...
With oDetails
    .ID = NCStr(datarow("ID")) ' or just CStr, since it's not nullable
    .BuildingNo = NCStr(datarow("BuildingNo"))
End With
\$\endgroup\$
3
\$\begingroup\$

A couple of things the other answer hasn't addressed.

  • Using a variable here seems a little superfluous.

            Dim sSQL As String
            sSQL = "spGetProperty"
            dsNames.SelectCommand = sSQL
    

    You could shorten this up a good bit by dropping the variable and just using the string literal. It's short enough that you're good there.

    dsNames.SelectCommand = "spGetProperty"
    

    If you want to keep the variable, you can initialize it when you declare it like this.

    Dim sSQL As String = "spGetProperty"
    dsNames.SelectCommand = sSQL
    

    Of course, if you go that route, you should use a constant rather than a variable, because you'll never be programmatically changing the value.

    Const sSQL As String = "spGetProperty"
    
  • Prefer String.Empty to Empty quotes.

                With oDetails
                    .ID= datarow("ID").ToString      
                    .BuildingNo = IIf(datarow("BuildingNo").Equals(DBNull.Value), "", datarow("BuildingNo").ToString)
                End With
    

    Writing the IIf with String.Empty instead makes it clear that the empty quotes was not a mistake by the previous developer.

  • People really frown on Systems Hungarian. You really shouldn't prefix all of your variables with their data type. What if the type changes later? Then either you have to let the code lie to you, or spend your time updating variable names. It's senseless. Don't do it.

\$\endgroup\$
0
\$\begingroup\$

Just a Little memento: IIF Will evaluate both statements.

Which can sometimes give you a surprise !

If there are some side effects this will totally kill You - Or at least me in 1998.

Use as said before; IF(True,"Yes","No").

Don't use IIF. Just Do not !

\$\endgroup\$
  • \$\begingroup\$ Now I can make Comments ! So can I now make above to a comment ? \$\endgroup\$ – Yay Jan 22 '15 at 10:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.