6

Over my programming career I formed a habit to introduce a flag variable that indicates that the first comparison has occured, just like Msft does in its linq Max() extension method implementation

public static int Max(this IEnumerable<int> source)
{
    if (source == null)
    {
        throw Error.ArgumentNull("source");
    }
    int num = 0;
    bool flag = false;
    foreach (int num2 in source)
    {
        if (flag)
        {
            if (num2 > num)
            {
                num = num2;
            }
        }
        else
        {
            num = num2;
            flag = true;
        }
    }
    if (!flag)
    {
        throw Error.NoElements();
    }
    return num;
}

However I have met some heretics lately, who implement this by just starting with the first element and assigning it to result, and oh no - it turned out that STL and Java authors have preferred the latter method.

Java:

public static <T extends Object & Comparable<? super T>> T max(Collection<? extends T> coll) {
Iterator<? extends T> i = coll.iterator();
T candidate = i.next();

    while (i.hasNext()) {
    T next = i.next();
    if (next.compareTo(candidate) > 0)
    candidate = next;
}
return candidate;
}

STL:

template<class _FwdIt> inline
_FwdIt _Max_element(_FwdIt _First, _FwdIt _Last)
{   // find largest element, using operator<
_FwdIt _Found = _First;
if (_First != _Last)
    for (; ++_First != _Last; )
        if (_DEBUG_LT(*_Found, *_First))
            _Found = _First;
return (_Found);
}

Are there any preferences between one method or another? Are there any historical reasons for this? Is one method more dangerous than another?

UPDATE: Please note, that the java version is capable of throwing exceptions with the line T candidate = i.next();and it's possible to get the same behavior in C# with the guard clause as it's shown in the second part of @ratchet freak's answer.

6
  • 4
    "Your" approach has about twice as much code (which also means more reading, which IMHO decreases readability) and no technical advantage. That settles it for me...
    – user7043
    Commented Nov 15, 2011 at 20:50
  • I find it a bit more declarative :) Commented Nov 15, 2011 at 21:02
  • 2
    Your approach also introduces an unnecessary branch inside the loop. Like @delnan said, there's no technical advantage to justify this. Commented Nov 15, 2011 at 22:05
  • 1
    The first snippet seems to be the unfortunate result of using foreach, so you can't start from the second item (although it should be possible?). - In C++, you could also trivially throw an exception if _First == _Last initially (except C++ folks don't like throwing exceptions around for things, which you could have just as well tested yourself before calling the function (or after obtaining the result).
    – UncleBens
    Commented Nov 16, 2011 at 22:57
  • why not use (or see source code) of LINQ source.Max() ? Commented Aug 16, 2014 at 21:23

3 Answers 3

2

to be totally radical you can do:

public static int Max(this IEnumerable<int> source)
{
    if (source == null)
    {
        throw Error.ArgumentNull("source");
    }

    int num = int.MinValue;
    foreach (int num2 in source)
    {
        if (num2 > num)
        {
            num = num2;
        }
    }
    return num;
}

by defining the max of a empty set to be int.Minvalue

but it is possible to do the java/stl approach with c#

public static int Max(this IEnumerable<int> source)
{
    if (source == null)
    {
        throw Error.ArgumentNull("source");
    }
    if(!source.MoveNext()){
        throw throw Error.NoElements();
    }
    int num = source.Current;
    foreach (int num2 in source)
    {
        if (num2 > num)
        {
            num = num2;
        }
    }
    return num;
}

though YMMV whether it's clearer or not

3
  • The MinValue approach is a good one for a simple case. But a generally useful Max() function will be generic and use an interface such as .NET's IComparable for the greater-than comparison, and it's a bit hard to know what initial value to use for a generic Max(). Commented Nov 15, 2011 at 23:27
  • 1
    that can be accomplished by explicitly giving a MinValue (not always feasible I know) Commented Nov 16, 2011 at 0:41
  • I'll mark this as accepted because the second part proves that it's possible to do the check for the empty list with a guard clause without using any flags, however I don't think that the first part is nice because it involves binary arithmetic. Commented Nov 17, 2011 at 7:14
5

There may well be preferences for one or the other; but one key distinction here is that the methods are not equivalent in semantics. Particularly in their handling of empty lists.

The LINQ version must return a value, and therefore if the list is empty throws an exception.

The C++ version, on the other hand, returns an iterator. That iterator may well point to the end() of the collection, and so any call has to be checked as to non-end-ness before using the result.

So the answer here is really more about how you expect to use the function, rather than its implementation. Danger is in the eye of the beholder. Exceptions vs. return and expectations are likely appropriate to their respective implementation philosophies.

1

Personally I've never developed a pattern to always do one way vs. doing it another way. For me it always boils to the fact that given everything else constant, more lines of code and more local variables increase the complexity of the code. Not by much but little by little, we end up with functions that get discussed next to the water cooler.

So I always pick approach which minimizes the use of local variables and use of conditionals whenever possible, given that I can shrink the code without sacrificing any clarity. But clarity would always win over using an extra local variable.

In the example you provided, I would probably code it without using the flag, but in the end it comes down to personal style so I wouldn't say it's wrong one way or the other.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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