[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

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

---

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 -

Quote:

---

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

---

Quote:

---

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!

Quote:

---

It's more succinctly written with regex:

---

Thats definitely more succinct, but is it more robust?

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

In that case +1 to your solution :)

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

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

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