current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 1 down vote favorite

I have a byte array and a List of a custom object ByteRule and I'm needing to loop around and perform an operation on both but only as long as one of both is available, the original method is as follows:

private final List<ByteRule> rules;

@Override
public boolean match(final InetAddress addr)
{
    final byte[] bytes = addr.getAddress();
    for(int i = 0; i < this.rules.size() && i < bytes.length; i++)
    {
        final int value = bytes[i] < 0 ? (int) bytes[i] + 256 : bytes[i];
        if(!this.rules.get(i).match(value))
        {
            return false;
        }
    }
    return true;
}

Obviously the this.rules.get(i) on every iteration is inefficient, however as I need to maintain a counter, an enhanced for loop means keeping a separate counter and performing a check on it every time like so (list declaration skipped for space):

@Override
public boolean match(final InetAddress addr)
{
    final byte[] bytes = addr.getAddress();
    int counter = 0;
    for(final ByteRule rule : this.rules)
    {
        if(counter + 1 == bytes.length) break; //In case there aren't enough bytes
        final int value = bytes[counter] < 0 ? (int) bytes[counter] + 256 : bytes[counter];
        if(!rule.match(value))
        {
            return false;
        }
        counter++;
    }
    return true;
}

Is there a cleaner/more efficient solution to this problem that I'm missing?

share|improve this question
    
Obviously the this.rules.get(i) on every iteration is inefficient That is not obvious to me at all. – abuzittin gillifirca Jul 24 '13 at 6:36
1  
This is how you can get rid of the ternary expression. Bitwise AND it with 0xFF. – abuzittin gillifirca Jul 24 '13 at 6:44
up vote 2 down vote accepted

I would be happy with:

@Override
public boolean match(final InetAddress inetAddress)
{
    final byte[] bytes = inetAddress.getAddress();
    int end = Math.min(this.rules.size(), bytes.length);
    for(int i = 0; i < end; i++)
    {
        final int adressPart = bytes[i] & 0xFF; //byte range from -128 to 127, &0xFF makes it unsigned, therefore positive
        if(!this.rules.get(i).match(adressPart))
            return false;
    }
    return true;
}

If you think, the this.rules.get(i), then test it with a profiler. I would be surprised if this line is the reason for any slowdown, because the length of a InetAddress is probably quite limited and most likely implemented as an ArrayList annyway. If it really matters, you can change it to:

@Override
public boolean match(final InetAddress inetAddress)
{
    final byte[] bytes = inetAddress.getAddress();
    ByteRule[] byteRulesArray = rules.toArray(new String[rules.size()]);
    int end = Math.min(this.rules.size(), bytes.length);
    for(int i = 0; i < end; i++)
    {
        final int adressPart = bytes[i] & 0xFF; //byte range from -128 to 127, &0xFF makes it unsigned, therefore positive
        if(!byteRulesArray[i].match(adressPart))
            return false;
    }
    return true;
}

or if you even fear the array creation, you can use an iterator:

@Override
public boolean match(final InetAddress inetAddress)
{
    final byte[] bytes = inetAddress.getAddress();
    Iterator<ByteRule> rulesIterator = rules.iterator();
    int end = Math.min(this.rules.size(), bytes.length);
    for(int i = 0; i < end; i++)
    {
        final int adressPart = bytes[i] & 0xFF; //byte range from -128 to 127, &0xFF makes it unsigned, therefore positive
        if(!rulesIterator.next.match(adressPart))
            return false;
    }
    return true;
}

Hint: In theory, the iterator solution would be faster than the array creation. However in the real world, because of hardware reasons, the array creation will, most likely, be faster.

share|improve this answer
    
I love the third solution, thank you very much for your help. – MrLore Jul 24 '13 at 20:35

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.