2
\$\begingroup\$

The function takes 2 arguments: array: int[], cap: int

Members would be ranged from 0 to cap.

e.g.

array = [0,0,0,2,6,3,0,4,2,4,0]
cap = 6

The return value is a new array of same length based on these rules:

  1. The function can only change values of consecutive elements which are > 0. i.e. any where the original array has 0, the new array has 0 as well.

  2. In a consecutive series of elements which are > 0, if the maximum of it equals cap, then in the new array they are changed to 1,1,...,1,cap of the same length. e.g. 0,2,6,4,0 -> 0,1,1,6,0

  3. In a consecutive series of elements which are > 0, if the maximum of it is smaller than cap, then the elements in the new array are mapped to either 1 or maximum (if the element value < maximum then 1, otherwise maximum ) . e.g. 0,2,5,5,0 -> 0,1,5,5,0

So the sample array would map to [0,0,0,1,1,6,0,4,1,4,0]

The following are my code which are implemented in both languages, it surprises me that F# takes same lines as C#.

I would like to suggestions to improve either language.

F#

open System

let private reorder cap (array: int[]) =
    if Array.isEmpty array then
        array
    else
        let max = array |> Array.max
        if max = cap then
            let newArray = Array.create array.Length 1
            newArray.[array.Length-1] <- cap
            newArray
        else
            array |> Array.map (fun elem -> if elem = max then max else 1)

let private getZeroes todo = 
    todo |> Seq.takeWhile (fun elem -> elem = 0)
         |> Seq.toArray

let private getNumbers todo cap =
    todo |> Seq.takeWhile (fun elem -> elem <> 0)
         |> Seq.toArray
         |> reorder cap

let GetEquivalentPermutation (array: int[], cap) =

    let rec joinParts finished todo =
        if Seq.isEmpty todo then finished |> Seq.toArray
        else
            let zeroes = getZeroes todo
            let nextTodo = todo |> Seq.skip zeroes.Length
            let numbers = getNumbers nextTodo cap
            let finalTodo = nextTodo |> Seq.skip numbers.Length
            let newFinished = Seq.append (Seq.append finished zeroes) numbers
            joinParts newFinished finalTodo

    joinParts [] array

C#

    static int[] EquivalentOutput(int cap, int[] permutation)
    {

        IEnumerable<int> right = permutation;
        List<int> equivalentLine = new List<int>();

        while (right.Any())
        {
            var leftArray = right.TakeWhile(x => x != 0).ToArray();

            List<int> leftEquivalent;
            if (leftArray.Length == 0)
            {
                leftEquivalent = new List<int>();
            }
            else if (leftArray.Length != 1)
            {
                int max = leftArray.Max();
                if (max == cap)
                {
                    leftEquivalent = leftArray.Take(leftArray.Length - 1).Select(x => 1).ToList();
                    leftEquivalent.Add(cap);
                }
                else
                {
                    leftEquivalent = leftArray.Select(x => x < max ? 1 : max).ToList();
                }
            }
            else
            {
                leftEquivalent = leftArray.ToList();
            }
            equivalentLine.AddRange(leftEquivalent);
            equivalentLine.AddRange(right.SkipWhile(x => x != 0).TakeWhile(x => x == 0));
            right = right.SkipWhile(x => x != 0).SkipWhile(x => x == 0);

        }
        return equivalentLine.ToArray();
    }
\$\endgroup\$

closed as unclear what you're asking by Malachi, Jeff Vanzella, rolfl, Mathieu Guindon, Simon Forsberg Nov 27 '13 at 20:31

Please clarify your specific problem or add additional details to highlight exactly what you need. As it's currently written, it’s hard to tell exactly what you're asking. See the How to Ask page for help clarifying this question. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ What is your question? Are you wanting this improved in F#, C# or both? What type of improvements are you looking for: performance, readability? \$\endgroup\$ – Gene S Oct 24 '12 at 14:39
  • \$\begingroup\$ @GeneS either will do. Yes, readability or elegance or conciseness \$\endgroup\$ – colinfang Oct 24 '12 at 15:00
  • \$\begingroup\$ please separate these into two questions, either you want a review in C# or you want a review in F#, pick one \$\endgroup\$ – Malachi Nov 27 '13 at 16:46
2
\$\begingroup\$

I think you should separate your concerns: first split the array into groups, then process each group separately and finally combine them back into a single array.

I think doing this results in code that is easier to understand:

private static IEnumerable<List<int>> SplitInGroups(int[] permutation)
{
    var result = new List<int>();

    foreach (var i in permutation)
    {
        // is this a new series?
        if (result.Any() && ((result.Last() == 0) != (i == 0)))
        {
            yield return result;
            result = new List<int>();
        }
        result.Add(i);
    }

    if (result.Any())
        yield return result;
}

private static IEnumerable<int> ProcessGroup(int cap, List<int> group)
{
    // testing the first element is enough
    if (group[0] == 0)
        return group;

    int max = group.Max();
    if (max == cap)
        return Enumerable.Repeat(1, group.Count - 1).Concat(new[] { max });

    return group.Select(i => i == max ? max : 1);
}

private static int[] EquivalentOutput(int cap, int[] permutation)
{
    return SplitInGroups(permutation).SelectMany(g => ProcessGroup(cap, g)).ToArray();
}
\$\endgroup\$
  • \$\begingroup\$ Definitely creative (not in a bad way) but not sure I agree it is easier to understand. \$\endgroup\$ – Gene S Oct 29 '12 at 14:37
2
\$\begingroup\$

Seems you might be overusing Linq since you are manipulating an array. Here is an example that to me is a bit more readable (although the readability could be improved further) and it is faster.

private static int[] EquivalentOutput(int cap, int[] permutation)
{
    int[] copy = permutation.ToArray();

    for (int i = 0; i < copy.Length; i++)
    {
        if (copy[i] == 0)
            continue;

        var consecutiveElements = copy.Skip(i).TakeWhile(x => x != 0).ToArray();
        var max = consecutiveElements.Max();
        if (max == cap)
        {
            for (int j = 0; j < consecutiveElements.Length - 1; j++)
                copy[i++] = 1;
            copy[i++] = cap;
        }
        else
        {
            for (int j = 0; j < consecutiveElements.Length; j++)
                copy[i] = copy[i++] == max ? max : 1;
        }
    }
    return copy;
}

Update: @svick pointed out that it feels a bit "weird" changing the loop variable inside the loop so I am providing an alternative which some people will consider easier to read...

private static int[] EquivalentOutput(int cap, int[] permutation)
{
    int[] copy = permutation.ToArray();

    int i = 0;
    while (i < copy.Length)
    {
        if (copy[i] > 0)
        {
            var consecutiveElements = copy.Skip(i).TakeWhile(x => x != 0).ToArray();
            var max = consecutiveElements.Max();
            if (max == cap)
            {
                for (int j = 0; j < consecutiveElements.Length - 1; j++)
                    copy[i++] = 1;
                copy[i++] = cap;
            }
            else
            {
                for (int j = 0; j < consecutiveElements.Length; j++)
                    copy[i] = copy[i++] == max ? max : 1;
            }
        }

        i++;
    }
    return copy;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I think you should use cast to int[] instead of as. If you make an error, you want to get an exception, not null. Also, modifying the loop variable inside the loop feels weird to me. \$\endgroup\$ – svick Oct 24 '12 at 18:35
  • \$\begingroup\$ why do u use .Clone() instead of .ToArray() \$\endgroup\$ – colinfang Oct 29 '12 at 10:27
  • \$\begingroup\$ @colingfang Wasn't thinking of that. .ToArray() is much better. Changing code to reflect this. \$\endgroup\$ – Gene S Oct 29 '12 at 14:23
  • \$\begingroup\$ @svick Used @colinfant suggestion so cast no longer necessary. As far as modifying the loop variable inside the loop...It performs well and I do not think it lessens the readability much. The for loop could be changed to a while loop if you think that improves the readability. \$\endgroup\$ – Gene S Oct 29 '12 at 14:25

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