1
\$\begingroup\$

I have written this code which takes a one dimensional array, for example:

{"abc","ade","sss","fgw","asd","lka","o"}

and turns it into an array of arrays, combining the elements which do not meet the condition.

For the condition m=>!m.Contains("a") for example, the output will be:

{{"abc","ade"},{"sss"},{"fgw"},{"asd","lka"},{"o"}}

The code works, but i suspect there is a linq way to do it, and refrain from using temporary variables that i dislike.

    public static string[][] combineBy(this string[] inp,Func<string,bool> f)
    {
        List<string[]> holdElements = new List<string[]>();
        while (inp.Length!=0)
        {
            if (f.Invoke(inp[0]))
            {
                holdElements.Add(new string[] { inp[0] });
                inp = inp.Skip(1).ToArray();
            }
            else
            {
                var toAdd = inp.TakeWhile(n => !f.Invoke(n));
                holdElements.Add(toAdd.ToArray());
                inp = inp.Skip(toAdd.Count()).ToArray();
            }
        }
        return holdElements.ToArray();
    }
\$\endgroup\$
9
  • \$\begingroup\$ You dislike temporary variables but you yourself are using them in var toAdd = ... instead of holdElements.Add(inp.TakeWhile(n => !f.Invoke(n))); ;-P which is inefficient anyway because you first enumerate the TakeWhile into an array and then you Count() instead of ToArray it in the first place and use the Length later. \$\endgroup\$ – t3chb0t Jul 26 '16 at 10:33
  • \$\begingroup\$ Seems hypocritical but it was meant for debugging and readability reasons, I didn't succeed in boiling down the code more than that tho. \$\endgroup\$ – downrep_nation Jul 26 '16 at 10:35
  • \$\begingroup\$ Just as a curiosity: I think this can be implemented in terms of codereview.stackexchange.com/questions/134363/…. The predicate, it seems, would be value => !value.contains("a"). In fact, perhaps, combineBy(list, predicate) is generally SplitBeforeIf(list, val => !predicate(val)). \$\endgroup\$ – Heman Gandhi Jul 26 '16 at 15:38
  • 2
    \$\begingroup\$ Please don't edit your question to incorporate changes from answers. It breaks the Q+A nature of the site. \$\endgroup\$ – forsvarir Jul 26 '16 at 16:27
  • \$\begingroup\$ I didnt incorporate changes from answers whatsoever. the code was simply not doing what i said it should because of a typo which is against the rules of this site \$\endgroup\$ – downrep_nation Jul 26 '16 at 16:29
7
\$\begingroup\$

Some general remarks about your code:

  • combineBy use PascalCase for method names
  • f it's not a good parameter name, if it's a condition function they are usualy named predicate
  • predicate.Invoke(...) - you can call it directly with predicate(...)
  • values.TakeWhile(n => !predicate(n)) - do not enumerate it twice, call ToArray and use it in the subsequent lines
  • if you like it the LINQ way you can write values.Any() instead of values.Length != 0 and values.First() instead of values[0] also this new string[] { inp[0] } could be turned into values.Take(1).ToArray()
  • don't use abbreviations for variables like inp or f, use full names like values and predicate

x

public static string[][] CombineBy(this string[] values, Func<string, bool> predicate)
{
    var result = new List<string[]>();
    while (values.Any())
    {
        if (predicate(values.First()))
        {
            result.Add(values.Take(1).ToArray());
            values = values.Skip(1).ToArray();
        }
        else
        {
            var toAdd = values.TakeWhile(n => !predicate(n)).ToArray();
            result .Add(toAdd);
            values = values.Skip(toAdd.Length).ToArray();
        }
    }
    return result.ToArray();
}

Unfortuantely your code doesn't seem to work and returns invalid results. This should do:

public static List<List<string>> CombineBy(this string[] values, Func<string, bool> predicate)
{
    var result = new List<List<string>>();

    foreach (var value in values)
    {
        if (predicate(value))
        {
            if (!result.Any() || !predicate(result.Last().Last()))
            {
                result.Add(new List<string>());
            }
            result.Last().Add(value);
        }
        else
        {
            result.Add(new List<string> { value });
        }
    }

    return result;
}

or even without loops:

public static List<List<string>> CombineBy3(this string[] values, Func<string, bool> predicate)
{
    var result = values.Aggregate(new List<List<string>>(), (current, next) =>
    {
        if (predicate(next))
        {
            if (!current.Any() || !predicate(current.Last().Last()))
            {
                current.Add(new List<string>());
            }
            current.Last().Add(next);
        }
        else
        {
            current.Add(new List<string> { next });
        }
        return current;
    });
    return result;
}
\$\endgroup\$
3
  • 2
    \$\begingroup\$ If you're going this far you may as well make it generic. \$\endgroup\$ – Der Kommissar Jul 26 '16 at 16:58
  • \$\begingroup\$ @EBrown right CombineBy<T> and string --> T + ;-) \$\endgroup\$ – t3chb0t Jul 26 '16 at 17:00
  • \$\begingroup\$ You got it. :) Then it's much more robust and usable. \$\endgroup\$ – Der Kommissar Jul 26 '16 at 17:03
4
\$\begingroup\$

I hate to break it to you but your code doesn't work. For the input you've given, your code returns:

{ {"abc"}, {"ade"}, {"sss", "fgw"}, {"asd"}, {"lka"}, {"o"} }

Which is clearly incorrect. Your code is also needlessly complex - you should be able to do this with one iteration through the source array:

public static IEnumerable<IEnumerable<string>> CombineBy(this string[] input, Func<string, bool> predicate)
{
    if (input == null) 
    {
        throw new ArgumentNullException(nameof(input));
    }
    return CombineByCore(input, predicate);
}

private static IEnumerable<IEnumerable<string>> CombineByCore(string[] input, Func<string, bool> predicate)
{
    var buffer = new List<string>();
    foreach (var inputString in input)
    {
        if (predicate(inputString))
        {
            buffer.Add(inputString);
        }
        else
        {
            if (buffer.Any())
            {
                yield return buffer;
                buffer = new List<string>();
            }
            yield return new List<string>() { inputString };
        }
    }
    if (buffer.Any())
    {
        yield return buffer;
    }
}
\$\endgroup\$
6
  • 1
    \$\begingroup\$ About the null check, see: stackoverflow.com/documentation/c%23/61/yield-keyword/279/… I made the same mistake :-( \$\endgroup\$ – Heslacher Jul 26 '16 at 11:03
  • \$\begingroup\$ @Heslacher - every day is a school day! Thanks for that - will update my answer \$\endgroup\$ – RobH Jul 26 '16 at 11:05
  • 1
    \$\begingroup\$ "every day is a school day" that what I thought some days ago as well. \$\endgroup\$ – Heslacher Jul 26 '16 at 11:05
  • \$\begingroup\$ Didn't mean to remove the null check but have another method doing it and then call the method which is yielding \$\endgroup\$ – Heslacher Jul 26 '16 at 11:06
  • 1
    \$\begingroup\$ I was being lazy :) \$\endgroup\$ – RobH Jul 26 '16 at 11:08
1
\$\begingroup\$

It's hard to find a "linq way" to accomplish that... However, you could use the GroupBy method with 2 helper variables:

public static IEnumerable<IEnumerable<string>> CombineBy(this IEnumerable<string> input, Func<string, bool> prediction)
{
    bool? prevPredict = null;
    int group = 0;
    return input.GroupBy(i =>
    {
        var predict = prediction(i);
        if (!predict || predict != prevPredict)
            group++;
        prevPredict = predict;
        return group;
    });
}
\$\endgroup\$
5
  • \$\begingroup\$ This is nice ;-] \$\endgroup\$ – t3chb0t Jul 26 '16 at 15:08
  • \$\begingroup\$ You call a string[] a list ... well, well, well... this needs a code review :-P \$\endgroup\$ – t3chb0t Jul 26 '16 at 15:11
  • \$\begingroup\$ Shame on me! Improved the answer ;) \$\endgroup\$ – JanDotNet Jul 26 '16 at 15:17
  • \$\begingroup\$ you got a typo, its accomplish. also it doesnt work because it groups variables that return true to the predicate \$\endgroup\$ – downrep_nation Jul 26 '16 at 16:18
  • \$\begingroup\$ @downrep_nation: Ok... than it becomes even more complex (see updated answer). \$\endgroup\$ – JanDotNet Jul 26 '16 at 16:31

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.