Results 1 to 9 of 9

Thread: [RESOLVED] Can this code be improved?

  1. #1

    Thread Starter
    Frenzied Member
    Join Date
    May 2002
    Posts
    1,602

    Resolved [RESOLVED] Can this code be improved?

    I was doing code reviews and stumbled upon this code

    Code:
     Private Function GetServerNameFromOracleConnectionString(connStringKey As String) As String
            Dim connString = ConfigurationManager.ConnectionStrings(connStringKey).ConnectionString
    
            If Not String.IsNullOrWhiteSpace(connString) Then
                Try
                    Dim startIndex = connString.LastIndexOf("SERVICE_NAME")
                    Dim indexofEqual = connString.IndexOf("=", startIndex) + 1
                    Dim indexofParenthesis = connString.IndexOf(")", startIndex)
                    Return connString.Substring(indexofEqual, indexofParenthesis - indexofEqual).Trim()
                Catch ex As Exception
                    Debug.WriteLine("Couldn't find the server name")
                    Return String.Empty
                End Try
            End If
            Return String.Empty
        End Function
    Is is used in a service form where the oracle instance is displayed, I guess mainly for trouble shooting purposes.

    I find some good things about it

    * naming
    * try-catch that prevents the app from throwing exception if the string parsing fails someone (but bad because no logging is done about it).

    and some bad:

    * magic strings
    * maybe too many lines of code and local members, can a lambda help to shorten the code and still maintain readability?
    * the code doesn't feel all that robust

    What do you think? Can it be improved somehow?

    /H

  2. #2
    eXtreme Programmer .paul.'s Avatar
    Join Date
    May 2007
    Location
    Chelmsford UK
    Posts
    25,464

    Re: Can this code be improved?

    It's more succinctly written with regex:

    Code:
    Imports System.Text.RegularExpressions
    
    Public Class Form1
    
        Private Sub Form1_Load(ByVal sender As System.Object, ByVal e As System.EventArgs) Handles MyBase.Load
            MsgBox(GetServerNameFromOracleConnectionString("SERVER=(DESCRIPTION=(ADDRESS=(PROTOCOL=TCP)(HOST=MyHost)(PORT=MyPort))(CONNECT_DATA=(SERVICE_NAME=MyOracleSID)));uid=myUsername;pwd=myPassword;"))
        End Sub
    
        Private Function GetServerNameFromOracleConnectionString(ByVal connStringKey As String) As String
            Dim m As Match = New Regex("SERVICE_NAME=(.+?)\)").Match(connStringKey)
            Return m.Groups(1).Value
        End Function
    
    End Class

  3. #3
    Superbly Moderated NeedSomeAnswers's Avatar
    Join Date
    Jun 2002
    Location
    Manchester uk
    Posts
    2,660

    Re: Can this code be improved?

    To be honest i think you have already named the things i would change.

    Put the "SERVICE_NAME" in a constant

    I would also put this line -

    Dim connString = ConfigurationManager.ConnectionStrings(connStringKey).ConnectionString
    inside your try catch as there is no reason why not to and it is then covered by your error handling too.

    So something like this -

    Code:
    Private Function GetServerNameFromOracleConnectionString(connStringKey As String) As String
            Const ServiceName As String = "SERVICE_NAME"
    
            Try
                Dim connString = ConfigurationManager.ConnectionStrings(connStringKey).ConnectionString
                If Not String.IsNullOrEmpty(connString) Then
                    Dim startIndex = connString.LastIndexOf(ServiceName)
                    Dim indexofEqual = connString.IndexOf("=", startIndex) + 1
                    Dim indexofParenthesis = connString.IndexOf(")", startIndex)
                    Return connString.Substring(indexofEqual, indexofParenthesis - indexofEqual).Trim()
                Else
                    Debug.WriteLine("Couldn't find the server name")
                    Return String.Empty
                End If
            Catch ex As Exception
                Debug.WriteLine("Couldn't find the server name")
                Return String.Empty
            End Try
        End Function
    can a lambda help to shorten the code and still maintain readability?
    I just don't see what your would gain out of a lambda in this situation, and i think it would be less readable!
    Please Mark your Thread "Resolved", if the query is solved & Rate those who have helped you



  4. #4
    Superbly Moderated NeedSomeAnswers's Avatar
    Join Date
    Jun 2002
    Location
    Manchester uk
    Posts
    2,660

    Re: Can this code be improved?

    It's more succinctly written with regex:
    Thats definitely more succinct, but is it more robust?
    Please Mark your Thread "Resolved", if the query is solved & Rate those who have helped you



  5. #5
    eXtreme Programmer .paul.'s Avatar
    Join Date
    May 2007
    Location
    Chelmsford UK
    Posts
    25,464

    Re: Can this code be improved?

    The regex will either find the string you're looking for or "" if there's no match. So yes, I'd say it's more robust

  6. #6
    Superbly Moderated NeedSomeAnswers's Avatar
    Join Date
    Jun 2002
    Location
    Manchester uk
    Posts
    2,660

    Re: Can this code be improved?

    In that case +1 to your solution
    Please Mark your Thread "Resolved", if the query is solved & Rate those who have helped you



  7. #7
    eXtreme Programmer .paul.'s Avatar
    Join Date
    May 2007
    Location
    Chelmsford UK
    Posts
    25,464

    Re: Can this code be improved?

    I missed the connString part. Use that with the regex...

  8. #8

    Thread Starter
    Frenzied Member
    Join Date
    May 2002
    Posts
    1,602

    Re: [RESOLVED] Can this code be improved?

    Thanks for the assistance. Actually, I didn't think much about the reg-exp but since it is used on some places in the app for validation, we could use it here as well. Good catch!

    One small issue with the regexp.

    It works great with the SERVICE_NAME=mydb)

    but not with SERVICE_NAME = mydb) <---- this is what we use

    /H
    Last edited by MrNorth; Nov 24th, 2015 at 08:33 AM.

  9. #9
    eXtreme Programmer .paul.'s Avatar
    Join Date
    May 2007
    Location
    Chelmsford UK
    Posts
    25,464

    Re: [RESOLVED] Can this code be improved?

    New Regex("SERVICE_NAME/s?=/s?(.+?)\)")

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •  



Click Here to Expand Forum to Full Width