Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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.

share|improve this question
3  
"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... –  delnan Nov 15 '11 at 20:50
    
I find it a bit more declarative :) –  Boris Treukhov Nov 15 '11 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. –  Sedate Alien Nov 15 '11 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 Nov 16 '11 at 22:57

3 Answers 3

up vote 2 down vote accepted

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

share|improve this answer
    
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(). –  Carson63000 Nov 15 '11 at 23:27
    
that can be accomplished by explicitly giving a MinValue (not always feasible I know) –  ratchet freak Nov 16 '11 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. –  Boris Treukhov Nov 17 '11 at 7:14

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.

share|improve this answer

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.

share|improve this answer

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.