HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Prettify this code - combine items in a list

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thiscombinecodeitemsprettifylist

Problem

Can anyone help me prettify this code? The idea is to combine adjacent time spans so

[0:00-1:00, 1:00-2:00, 3:00-4:00] becomes [0:00-2:00, 3:00-4:00]

private static IEnumerable CombineAdjacentSpans(IEnumerable spans)
{
    var combined = new List();
    var list = spans.OrderBy(s => s.Start).ToList();

    var candidate = list.FirstOrDefault();
    foreach (var span in list.Skip(1))
    {
        if (span.Start == candidate.End)
        {
            candidate.End = span.End;
            continue;
        }

        combined.Add(candidate);
        candidate = span;
    }

    if (candidate != null)
        combined.Add(candidate);

    return combined;
}

Solution

You can shorten this and get lazy evalution using yield:

private static IEnumerable CombineAdjacentSpans(this IEnumerable spans)
{
    Span current = null;
    foreach (var span in spans.OrderBy(s => s.Start)) {
        if (current == null) { current == span; }
        // I made this handle overlaps, but if you don't care about those
        // you can change the comparison to just ==
        else if (span.Start  current.End) {
            // current.End = span.End
            // it's probably better to make a new span rather than modifying the
            // current one here since that way this method doesn't modify elements
            // in the original list. If span is a struct, then modification would be
            // fine but we'd have to rely on a bool rather than checking against null
            current = new Span { Start = current.Start, End = span.End };
        } else {
            yield return current;
            current = span;
        }              
    }

    if (current != null) { yield return current; }
}


This way, something like mySpans.CombineAdjacentSpans().Take(3) will only do the work of combining the first 3 spans (although OrderBy will still have to look at all elements in the input sequence).

In response to Chris's comment, if you really want the last if statement inside the loop here's one way I can think of to do it:

private static IEnumerable CombineAdjacentSpans(this IEnumerable spans)
{
    Span current = null;
    // could also use Enumerable.Repeat() instead of new[] { ... }
    foreach (var span in spans.OrderBy(s => s.Start).Concat(new[] { default(Span) })) {
        if (current == null) { current == span; }
        // I made this handle overlaps, but if you don't care about those
        // you can change the comparison to just ==
        else if (span.Start  current.End) {
            // current.End = span.End
            // it's probably better to make a new span rather than modifying the
            // current one here since that way this method doesn't modify elements
            // in the original list. If span is a struct, then modification would be
            // fine but we'd have to rely on a bool rather than checking against null
            current = new Span { Start = current.Start, End = span.End };
        } else {
            yield return current;
            current = span;
        }              
    }
}

Code Snippets

private static IEnumerable<Span> CombineAdjacentSpans(this IEnumerable<Span> spans)
{
    Span current = null;
    foreach (var span in spans.OrderBy(s => s.Start)) {
        if (current == null) { current == span; }
        // I made this handle overlaps, but if you don't care about those
        // you can change the comparison to just ==
        else if (span.Start <= current.End && span.End > current.End) {
            // current.End = span.End
            // it's probably better to make a new span rather than modifying the
            // current one here since that way this method doesn't modify elements
            // in the original list. If span is a struct, then modification would be
            // fine but we'd have to rely on a bool rather than checking against null
            current = new Span { Start = current.Start, End = span.End };
        } else {
            yield return current;
            current = span;
        }              
    }

    if (current != null) { yield return current; }
}
private static IEnumerable<Span> CombineAdjacentSpans(this IEnumerable<Span> spans)
{
    Span current = null;
    // could also use Enumerable.Repeat() instead of new[] { ... }
    foreach (var span in spans.OrderBy(s => s.Start).Concat(new[] { default(Span) })) {
        if (current == null) { current == span; }
        // I made this handle overlaps, but if you don't care about those
        // you can change the comparison to just ==
        else if (span.Start <= current.End && span.End > current.End) {
            // current.End = span.End
            // it's probably better to make a new span rather than modifying the
            // current one here since that way this method doesn't modify elements
            // in the original list. If span is a struct, then modification would be
            // fine but we'd have to rely on a bool rather than checking against null
            current = new Span { Start = current.Start, End = span.End };
        } else {
            yield return current;
            current = span;
        }              
    }
}

Context

StackExchange Code Review Q#15264, answer score: 4

Revisions (0)

No revisions yet.