Convert algorithm from C# to VB.NET fail

Question

I'm trying to convert the following algorithm from C# to VB.NET and the VB.NET I have is not producing the same results as my C# algorithm, can someone tell me where I've gone wrong in my conversion?

public static IEnumerable<T[]> Combinations<T>(this IEnumerable<T> elements, int k)
{
    List<T[]> result = new List<T[]>();

    // single combination
    if (k == 0)
    {
        result.Add(new T[0]);
    }
    else
    {
        int current = 1;
        foreach (T element in elements)
        { 
            //combine each element with k-1 combinations of subsequent elements
            result.AddRange(elements
                .Skip(current++)
                .Combinations(k - 1)
                .Select(combination => (new T[] { element }).Concat(combination).ToArray())
                );
        }
    }
    return result;
}

This is what I've got in VB.NET:

<Extension()>
Public Function Combinations(Of T)(ByRef elements As IEnumerable(Of T), ByVal k As Integer) As IEnumerable(Of T())

    Dim result As New List(Of T())()

    'single combination'
    If k = 0 Then
        result.Add(New T(-1) {})
    Else
        Dim current As Integer = 0

        For Each element As T In elements
            'combine each element with k - 1 combinations of subsequent elements'
            Dim local As T = element
            result.AddRange(elements.Skip(current = current + 1).Combinations(k - 1).Select(Function(combs) (New T() {local}).Concat(combs).ToArray()))
        Next
    End If

    Return result
End Function

Something is wrong, but I'm not sure what, I'm guessing the problem is somewhere in the lambda.

Can anybody point out what I've done wrong with my conversion?

Answer 1

There’s no need for the in-place increment of current at all, as far as I can see. Just increment it after the Linq expression. On the other hand, you should initialise current with 1, same as in C#.

Furthermore, your “base case” is a bit weird; you can just write this:

result.Add(New T() { })

No need for the -1.

Answer 2

I'm no expert on VB at all, but the problem might stem from either:

current = current + 1 is not the same as current++. It is (basically) the same as ++current. These have way different behavior.

Since VB doesn't support inline post-increment operators directly, the closest bug-free implementation I can think of is (in C#, since I don't know VB):

int current = 0;
Func<int> getCurrentThenIncrement = () =>
{
    int previous = current;
    current = current + 1;
    return previous;
};

// ...
.Skip(getCurrentThenIncrement())

Or, it could stem from:

Public Function Combinations(Of T)(ByRef elements ...

When I use .Net Reflector to look at it and "convert" it to VB, elements seems to be ByVal.

Answer 3

Use a code converter...

<System.Runtime.CompilerServices.Extension> _
Public Shared Function Combinations(Of T)(elements As IEnumerable(Of T), k As Integer) As IEnumerable(Of T())
    Dim result As New List(Of T())()

    ' single combination
    If k = 0 Then
        result.Add(New T(-1) {})
    Else
        Dim current As Integer = 1
        For Each element As T In elements
            'combine each element with k-1 combinations of subsequent elements

            result.AddRange(elements.Skip(System.Math.Max(System.Threading.Interlocked.Increment(current),current - 1)).Combinations(k - 1).[Select](Function(combination) (New T() {element}).Concat(combination).ToArray()))
        Next
    End If

    Return result
End Function

Answer 4

I don't have a compiler on hand to check, but I would convert it like this:

<Extension()>
Public Function Combinations(Of T)(elements As IEnumerable(Of T), k As Integer) As IEnumerable(Of T())

    Dim result As New List(Of T())()

    ' single combination
    If k = 0 Then
        result.Add(New T() {})
    Else
        Dim current As Integer = 1
        For Each element As T In elements
            'combine each element with k-1 combinations of subsequent elements
            result.AddRange(elements
                .Skip(PostfixInc(current))
                .Combinations(k - 1)
                .Select(Function(combination) (New T() { element }).Concat(combination).ToArray())
                )
        Next
    End If
    Return result

End Function

Private Function PostfixInc(ByRef value As Integer) As Integer

    Dim currentValue = value
    value += 1
    Return currentValue

End Function

That's as direct a conversion of each syntax element as I can think of right now. Once you have it working, then think about cleaning it up (e.g. try to remove side effects from the LINQ expression).

EDIT:

Differences from your code:

• New T(-1) {} should be New T() {}.
• current should be initialised to 1 not 0.
• Skip(current = current + 1) will not do what you want. In fact, it does nothing, because current = current + 1 is a boolean expression that will always evaluate to false. VB.NET will silently convert false to 0 if option strict is off, or throw a compiler error if option strict is on.