9
\$\begingroup\$

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();
}
//...
\$\endgroup\$
  • \$\begingroup\$ 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); \$\endgroup\$ – Markus May 27 '14 at 21:28
  • 1
    \$\begingroup\$ The method signature is incorrect. It should be private object FindStuff(); \$\endgroup\$ – Sandeep May 28 '14 at 9:24
11
\$\begingroup\$

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 object FindStuff()
{
    object previous = null;
    foreach (object current in array)
    {
        if (Condition(current))
        {
            return previous;
        }
        previous = current;
    }
    throw new ItemNotFoundException();
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Just a minor one - you're returning an object from a void method. \$\endgroup\$ – Coda Oct 14 '19 at 10:26
  • \$\begingroup\$ @Coda - thanks for noticing that. Fixed. \$\endgroup\$ – rolfl Oct 14 '19 at 13:33
3
\$\begingroup\$

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 :)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Markus May 28 '14 at 12:09
  • \$\begingroup\$ 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(). \$\endgroup\$ – NotPro May 28 '14 at 12:44
  • \$\begingroup\$ 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 ... \$\endgroup\$ – Markus May 28 '14 at 12:49
  • \$\begingroup\$ 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. \$\endgroup\$ – abuzittin gillifirca May 28 '14 at 14:39
  • \$\begingroup\$ 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];} \$\endgroup\$ – Markus May 28 '14 at 14:56
3
\$\begingroup\$

The question seems to be missing an important constraint in case there are repeating elements in the input collection that satisfy the condition given. Therefore, there is an assumption around the solutions provided that we are interested in the first encounter only.

How about a more general approach to return an IEnumerable collection so that the caller can decide what to take out of that such as the first, the last or the nth, etc.

    private static IEnumerable<object> Find(object[] array)
    {
        if (array == null) throw new ArgumentNullException(nameof(array));

        object previous = null;
        foreach (var current in array)
        {
            if (Predicate(current)) yield return previous;

            previous = current;
        }
    }

    private static bool Predicate(object o)
    {
        return o != null && (int)o == 2;
    }

There is also a null check for the input to fail fast. Predicate logic is irrelevant here.

Following is the test cases for the Find method:

    [Test]
    public void TestFind()
    {
        var items = Find(new object[] { 1, 2, 5, 2, 10, 2, 100, 2 });
        Assert.AreEqual(1, items.First());

        items = Find(new object[] { 0, 1, 1, 1, 1, 1 });
        Assert.IsEmpty(items);

        Assert.Throws<ArgumentNullException>(() => Find(null).ToList());
    }
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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