current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 8 down vote favorite

Any ideas to simplify this beauty; I would prefer a LinQ expression if possible:

private object[] array;
public abstract bool Condition(object o);
//...
private object FindStuff()
{
    for (int i = 0; i < array.Length; i++)
    {
        if (Condition(array[i]))
        {
            return i == 0 ? null : array[i-1];
        }
    }
    throw new ItemNotFoundException();
}
//...
share|improve this question
    
Using the assumption, that there is an object foo; as a non-valid data-placeholder, I have a new approach but I hate it even more: return array.Select((t, i) => Condition(t) ? i == 0 ? null : array[i - 1] : foo).First(e => e != foo); –  Markus 2 days ago
1  
The method signature is incorrect. It should be private object FindStuff(); –  Sandeep yesterday
    
Thanks @Sandeep. Fixed it above. –  Markus yesterday
add comment

2 Answers

up vote 11 down vote accepted

This code would benefit from a previous variable...... and it would also benefit from being real code, not this hypothetical example..... This code is also really short, so it's hard to simplify more.

Still, using a foreach is better than the indexed iterator, and the logic is more obvious with named variables, rather than indexes... so:

private object[] array;
public abstract bool Condition(object o);
//...
private void FindStuff()
{
    object previous = null;
    foreach (object current in array)
    {
        if (Condition(current))
        {
            return previous;
        }
        previous = current;
    }
    throw new ItemNotFoundException();
}
share|improve this answer
add comment
up vote 3 down vote

List<T> has FindIndex(Predicate<T>) which returns index or -1 when item was not found.

private static object FindStuff(List<object> list)
{
    int index = list.FindIndex(Condition);

    if (index < 0) throw new ItemNotFoundException();

    if (index == 0) return null;

    return list[index - 1];
}

private static bool Condition(object o)
{
    return true;
}

I don't like returning null, but whateva :)

share|improve this answer
    
Yes that's a nice solution as well ... but I would prefer a switch instead of the if-else. I like the other answer more, since it is working without any magic numbers. –  Markus yesterday
    
To be fair I also like rolfl solution more because it tells what you want to do but to be honestly I don't like an idea of creating function like this. How you will use it? :) try { FindStuff(); DealWithNullCase() } catch { }? This function smells very bad... I would probably at least split it in two functions bool PreviousExists(), bool Previous(). –  NotPro yesterday
    
You are right the sample is not very good! The snippet above is just an abstraction of my real issue. There is code before and after the loop and the method name is also different. My method is also not returning null, but a default value coming from the method's params. However, I'd have to explain a whole tree data structure concept to post the original method here ... –  Markus yesterday
    
else is not needed if the previous if statement ends with a return or throw. Also you should handle exceptional cases, such as negative return value from FindIndex, as close to the relevant location as possible. Also @Markus: 0 and 1 are not magical numbers. –  abuzittin gillifirca yesterday
    
Yes but -1 is ;) I've tested your code with a switch(index) {case -1: throw new Exception(); case 0: return null; default: return list[index-1];} –  Markus yesterday
add comment

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.