[VB.NET]GeoIP Using XML[Source]

[AceInfinity]

The method you have here is practically useless unless somebody else has your exact setup, you could have simplified this much further as well, and used parameters (that's what they are there for, and this is a prime example of where it could be useful to have some).

If you're going to release a source like this you should expect some feedback, whether it's good or bad.

I would have also used a class for this, another prime example of where it could be useful. Maybe not in your mind for something so simple, although what if now you have a program that keeps track of multiple geolocations? You're going to do that all manually?

What about multi-threading here?

Code:

For i = 0 To xmlnode.Count - 1

That would be the equivalent of For i = 0 To 0 here in this case lol

Also you have a stray line here which doesn't do anything:

Code:

xmlnode(i).ChildNodes.Item(0).InnerText.Trim()

Code:

Dim i As Integer

You don't have to do that here...

If you want to loop through all nodes and childnodes of those nodes you'd do something like this:

Code:

For i As Integer = 0 To xmlnode.Count - 1
    For c As Integer = 0 To xmlnode(i).ChildNodes.Count
        'Do something with xmlnode(i).ChildNodes.Item(c).InnerText
    Next
Next

Otherwise, since we know here that there is only 1 xmlnode (xmlnode(0)), then we could avoid this loop entirely as we also know how many childnodes there is. It really doesn't do you any good to use a loop if you're going to write all of the values out manually like that anyways lol. So you'd be better off just not using the loop.

If you want to use a loop though then you'd do what I suggested above.

I would have done this like so:

Code:

Private Function GetGeoLocation(IP As String) As GEOLocation
    Dim xmlDoc As New XmlDocument
    Dim nodeList As XmlNodeList

    xmldoc.Load("http://freegeoip.net/xml/" & IP)
    nodeList = xmldoc.GetElementsByTagName("Response")

    Dim node As XmlNode = nodeList(0)

    Dim GEOResults As New GEOLocation
    For i As Integer = 0 To 9
        Select Case i
            Case 0
                GEOResults.IP = node.ChildNodes(i).InnerText
            Case 1
                GEOResults.CountryCode = node.ChildNodes(i).InnerText
            Case 2
                GEOResults.CountryName = node.ChildNodes(i).InnerText
            Case 3
                GEOResults.RegionCode = node.ChildNodes(i).InnerText
            Case 4
                GEOResults.RegionName = node.ChildNodes(i).InnerText
            Case 5
                GEOResults.City = node.ChildNodes(i).InnerText
            Case 6
                GEOResults.ZipCode = node.ChildNodes(i).InnerText
            Case 7
                GEOResults.Latitude = node.ChildNodes(i).InnerText
            Case 8
                GEOResults.Longitude = node.ChildNodes(i).InnerText
            Case 9
                GEOResults.MetroCode = node.ChildNodes(i).InnerText
        End Select
    Next

    Return GEOResults
End Function

Public Class GEOLocation

    Private _IP As String
    Private _CountryCode As String
    Private _CountryName As String
    Private _RegionCode As String
    Private _RegionName As String
    Private _City As String
    Private _ZipCode As String
    Private _Latitude As String
    Private _Longitude As String
    Private _MetroCode As String

    Public Property IP() As String
        Get
            Return _IP
        End Get
        Set(ByVal value As String)
            _IP = value.Trim()
        End Set
    End Property

    Public Property CountryCode() As String
        Get
            Return _CountryCode
        End Get
        Set(ByVal value As String)
            _CountryCode = value.Trim()
        End Set
    End Property

    Public Property CountryName() As String
        Get
            Return _CountryName
        End Get
        Set(ByVal value As String)
            _CountryName = value.Trim()
        End Set
    End Property

    Public Property RegionCode() As String
        Get
            Return _RegionCode
        End Get
        Set(ByVal value As String)
            _RegionCode = value.Trim()
        End Set
    End Property

    Public Property RegionName() As String
        Get
            Return _RegionName
        End Get
        Set(ByVal value As String)
            _RegionName = value.Trim()
        End Set
    End Property

    Public Property City() As String
        Get
            Return _City
        End Get
        Set(ByVal value As String)
            _City = value.Trim()
        End Set
    End Property

    Public Property ZipCode() As String
        Get
            Return _ZipCode
        End Get
        Set(ByVal value As String)
            _ZipCode = value.Trim()
        End Set
    End Property

    Public Property Latitude() As String
        Get
            Return _Latitude
        End Get
        Set(ByVal value As String)
            _Latitude = value.Trim()
        End Set
    End Property

    Public Property Longitude() As String
        Get
            Return _Longitude
        End Get
        Set(ByVal value As String)
            _Longitude = value.Trim()
        End Set
    End Property

    Public Property MetroCode() As String
        Get
            Return _MetroCode
        End Get
        Set(ByVal value As String)
            _MetroCode = value.Trim()
        End Set
    End Property

End Class

Now say we could return multiple GeoLocations for some reason from this website, then I would create a List(Of GEOLocation) and return that from the function for each iteration through the nodes after all childnodes have been looped through. But that's not the case, so perhaps a loop here is overkill.

Regardless, there's still many anomalies in the code i've seen there... Not good.

One last thing, don't ever name your variables with the names of existing Classes from the .NET namspaces you've referenced.

VB is case-insensitive:

Code:

Dim xmlnode As XmlNodeList

XmlNode is already a Class: http://msdn.microsoft.com/en-us/library/...lnode.aspx

The reason i'm using a class with properties there is for encapsulation of the Trim() method, you could create a function to do whatever you want with the input strings before setting the values in stone, so this was just a demonstration, I wouldn't repeat that though if it's anything more complicated than Trim(), create a function to do it and return a modified value if you want.