-
Nov 24th, 2015, 03:35 AM
#1
Thread Starter
Frenzied Member
[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
-
Nov 24th, 2015, 03:51 AM
#2
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
- Coding Examples:
- Features:
- Online Games:
- Compiled Games:
-
Nov 24th, 2015, 04:26 AM
#3
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
-
Nov 24th, 2015, 04:28 AM
#4
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
-
Nov 24th, 2015, 04:53 AM
#5
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
- Coding Examples:
- Features:
- Online Games:
- Compiled Games:
-
Nov 24th, 2015, 05:33 AM
#6
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
-
Nov 24th, 2015, 05:46 AM
#7
Re: Can this code be improved?
I missed the connString part. Use that with the regex...
- Coding Examples:
- Features:
- Online Games:
- Compiled Games:
-
Nov 24th, 2015, 08:25 AM
#8
Thread Starter
Frenzied Member
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.
-
Nov 24th, 2015, 08:43 AM
#9
Re: [RESOLVED] Can this code be improved?
New Regex("SERVICE_NAME/s?=/s?(.+?)\)")
- Coding Examples:
- Features:
- Online Games:
- Compiled Games:
Posting Permissions
- You may not post new threads
- You may not post replies
- You may not post attachments
- You may not edit your posts
-
Forum Rules
|
Click Here to Expand Forum to Full Width
|