I was doing code reviews and stumbled upon this code
Is is used in a service form where the oracle instance is displayed, I guess mainly for trouble shooting purposes.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
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




Reply With Quote