current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite
1

I have a small assignment to read IP addresses and from a bunch of external text files identify which country the IP address comes from. I opted to use XmlReader and read the ranges from an XML file but unfortunately the performance has been quite abysmal (an average search time of around 7000 milliseconds).

Obviously serializing the ranges isn't an option as we're talking about 142,222 entries. Is there anything I can do to optimize my method or is there a quicker way to search this, without resorting to SQL?

class IPRangeRepository : IDisposable
    {
        public IPRangeRepository(string fileName)
        {
            this.fileName = fileName;
            xmlReader = XmlReader.Create(fileName);
        }

        private XmlReader xmlReader;
        private string fileName;

        public string GetIPCountry(IPAddress adress)
        {
            while (xmlReader.Read())
            {
                // Ok instantiate object and then do comparison... 
                if (xmlReader.Name == "IPRange") { 
                var ipRange = IPRange.TryParse(xmlReader["Value"]);
                if (ipRange.IsIPAddressInRange(adress)) return xmlReader["Country"];
                }
            }

            //Return empty string if we don't find anything
            return string.Empty;
        }

        public void Dispose()
        {
            xmlReader.Dispose();
        }
    }
share|improve this question
1  
Can you post a part of the xml file ? Is IsIPAddressInRange() an extension method ? –  Heslacher Oct 8 '14 at 15:17
1  
there isn't enough information here to help you speed this up. –  Malachi Oct 8 '14 at 15:27
    
sounds like you have been having issues with posting code, if you check this link it shows a couple of ways in which you can ready your code to be posted on Stack Exchange Sites, meta.codereview.stackexchange.com/a/1377/18427 –  Malachi Oct 9 '14 at 21:21

2 Answers 2

up vote 2 down vote

You should be able to load them all into memory and then query them there, that should be much faster. 150k entries shouldn't take more than 10 MB of memory, which for most applications should be okay.

share|improve this answer
    
I would definitely load them and keep them in memory, probably some search structure (dictionary, sorted list, tree or the like). –  firda Oct 9 '14 at 21:24
    
@firda Yeah, that would be my second step. But since a simple dictionary probably won't work here, I would start with a simple linear search, that should already improve performance significantly. –  svick Oct 10 '14 at 0:19
    
The ranges should be discrete. Sorted list or tree should do. –  firda Oct 10 '14 at 5:12
up vote 1 down vote

the only issues that I see with this code so far is formatting.

class IPRangeRepository : IDisposable
    {
        public IPRangeRepository(string fileName)
        {
            this.fileName = fileName;
            xmlReader = XmlReader.Create(fileName);
        }

This indentation is not standard, it should look like this

class IPRangeRepository : IDisposable
{
    public IPRangeRepository(string fileName)
    {
        this.fileName = fileName;
        xmlReader = XmlReader.Create(fileName);
    }

and your if statements inside the while loop are not easy to read.

        while (xmlReader.Read())
        {
            // Ok instantiate object and then do comparison... 
            if (xmlReader.Name == "IPRange") { 
            var ipRange = IPRange.TryParse(xmlReader["Value"]);
            if (ipRange.IsIPAddressInRange(adress)) return xmlReader["Country"];
            }
        }

this is how I prefer to write this code

while (xmlReader.Read())
{
    // Ok instantiate object and then do comparison... 
    if (xmlReader.Name == "IPRange") { 
        var ipRange = IPRange.TryParse(xmlReader["Value"]);
        if (ipRange.IsIPAddressInRange(adress)) 
        {
            return xmlReader["Country"];
        }
    }
}

the return is a little too complex for something that I would one line, and most of the time if I can one line something it's going to look better as a ternary statement, this isn't one of those instances.

Always indent code inside of any block.

share|improve this answer
    
These are usually copy-paste errors and problems with tabs here on SE (especially when mixed with spaces). –  firda Oct 9 '14 at 21:10
    
@firda I apologize in advance, but I have been editing posts on Code Review for quite some time, and the post has no sign of a copy paste error, this is the way that the user wrote the code. if you notice, even Jamal hasn't edited this post yet. –  Malachi Oct 9 '14 at 21:13
    
Of course I cannot be sure of that. Recently I have asked the author (elsewhere, different question) to correct the indentation and he did try but obviously had difficulties to get it right. I have used VIM and few tricks to correct it. I have explicitly stated there that I would do that automatically on SO, but it is more complicated here as I know it could be about codestyling. That is simply reason I usually leave it when it looks like copy-paste error. Again, no guarantee to be this or that. –  firda Oct 9 '14 at 21:16
    
I mean some author... probably not this one. But an hour ago or so, similar errors (but few } were outside so it was more obvious than here). –  firda Oct 9 '14 at 21:23

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.