4
\$\begingroup\$

I wish to have code that returns the center items of a 2d grid. Because I need an average value. The code I came up with does what I want (see the unit tests beneath it). But I think the code is ugly and have the feeling it can be optimized. However, I do not see a way. Could this code be optimized by your mind? If so I like to learn how. I am not that concerned with performance, rather readability of the code. I think it is too clunky now.

    public static IEnumerable<T> GetCenterItems<T>(this T[,] items)
    {
        var nrOfRows = items.GetLength(0);
        var nrOfColumns = items.GetLength(1);

        var itemsToReturn = new List<T>();
        if (nrOfRows.IsEven())
        {
            var startRow = nrOfRows / 2 - 1;
            if (nrOfColumns.IsEven())
            {
                var startColumn = nrOfColumns / 2 - 1;
                itemsToReturn.Add(items[startRow, startColumn]);
                itemsToReturn.Add(items[startRow, startColumn + 1]);
                itemsToReturn.Add(items[startRow + 1, startColumn]);
                itemsToReturn.Add(items[startRow + 1, startColumn + 1]);
            }
            else
            {
                var columnNr = (nrOfColumns / 2);
                itemsToReturn.Add(items[startRow, columnNr]);
                itemsToReturn.Add(items[startRow + 1, columnNr]);
            }
        }
        else
        {
            var rowNr = (nrOfRows / 2);
            if (nrOfColumns.IsEven())
            {
                var startColumn = (nrOfColumns / 2) - 1;
                itemsToReturn.Add(items[rowNr, startColumn]);
                itemsToReturn.Add(items[rowNr, startColumn + 1]);
            }
            else
            {
                var columnNr = (nrOfColumns / 2);
                itemsToReturn.Add(items[rowNr, columnNr]);
            }
        }

        return itemsToReturn;
    }

And the unit tests

    [Test]
    public void CorrectCenterItemsAreReturnedForEvenRowsAndEvenColumns()
    {
        //Given
        var array = new [,]
        {
            {1, 2, 3, 4 },
            {5, 6, 7, 8 },
            {9, 10, 11, 12 },
            {13, 14, 15, 16 },
            {17, 18, 19, 20 },
            {21, 22, 23, 24 },
        };
        var expectedCenterItems = new[]
        {
            10, 11, 14, 15
        };

        //When
        var actualCenterItems = array.GetCenterItems();

        //Then
        actualCenterItems.Should().BeEquivalentTo(expectedCenterItems);
    }

    [Test]
    public void CorrectCenterItemsAreReturnedForEvenRowsAndOddColumns()
    {
        //Given
        var array = new[,]
        {
            {1, 2, 3, 4, 25},
            {5, 6, 7, 8, 26 },
            {9, 10, 11, 12, 27 },
            {13, 14, 15, 16, 28 },
            {17, 18, 19, 20, 29 },
            {21, 22, 23, 24, 30 }
        };
        var expectedCenterItems = new[]
        {
            11, 15
        };

        //When
        var actualCenterItems = array.GetCenterItems();

        //Then
        actualCenterItems.Should().BeEquivalentTo(expectedCenterItems);
    }

    [Test]
    public void CorrectCenterItemsAreReturnedForOddRowsAndOddColumns()
    {
        //Given
        var array = new[,]
        {
            {1, 2, 3, 4, 25},
            {5, 6, 7, 8, 26 },
            {9, 10, 11, 12, 27 },
            {13, 14, 15, 16, 28 },
            {17, 18, 19, 20, 29 }
        };
        var expectedCenterItems = new[]
        {
            11
        };

        //When
        var actualCenterItems = array.GetCenterItems();

        //Then
        actualCenterItems.Should().BeEquivalentTo(expectedCenterItems);
    }

    [Test]
    public void CorrectCenterItemsAreReturnedForOddRowsAndEvenColumns()
    {
        //Given
        var array = new[,]
        {
            {1, 2, 3, 4},
            {5, 6, 7, 8 },
            {9, 10, 11, 12 },
            {13, 14, 15, 16 },
            {17, 18, 19, 20 }
        };
        var expectedCenterItems = new[]
        {
            10, 11
        };

        //When
        var actualCenterItems = array.GetCenterItems();

        //Then
        actualCenterItems.Should().BeEquivalentTo(expectedCenterItems);
    }
\$\endgroup\$
7
\$\begingroup\$

Personally, I'm not a fan of this at all. You're right that it's ugly, but we can fix that.

To boot, we're going to remove every single if statement here.

Let's talk about a quick mathematic property of what we're doing: we can get the midpoint of both sides of the array, then round up and/or down to find our values. Basically, if we get the midpoint, it will either be a decimal, or not. If it's a decimal, floor(val) and ceil(val) will give us the whole numbers on either side, and if it's not a decimal they'll just return the raw value.

var nrOfRows = items.GetLength(0);
var nrOfColumns = items.GetLength(1);

This is a good start, and I like what you did here. This is smart - put the things that are not the best to remember in variables, we're going to make one modification: divide by 2.0:

var nrOfRows = (items.GetLength(0) -1) / 2.0;
var nrOfColumns = (items.GetLength(1) -1) / 2.0;

Fun fact: this will promote the int that GetLength returns to a double, then do floating-point division.

We want this, because now we can instance four quick variables using Math.Floor and Math.Ceiling:

var fRow = (int)Math.Floor(nrOfRows);
var lRow = (int)Math.Ceiling(nrOfRows);
var fCol = (int)Math.Floor(nrOfColumns);
var lCol = (int)Math.Ceiling(nrOfColumns);

Easy-peasy. The next part is simple: add each combination of row, col to a List<Tuple<int, int>>. This is so that we can use LINQ to quickly build our final result:

var cells = new List<Tuple<int, int>> {
    new Tuple<int, int>(fRow, fCol),
    new Tuple<int, int>(fRow, lCol),
    new Tuple<int, int>(lRow, fCol),
    new Tuple<int, int>(lRow, lCol)
};

Ok, so this seems counter-intuitive, right? We're going to add all the coordinates, even though in the case of odd/odd, it'll be the same for each. Here's where LINQ comes in: using .Distinct() we can get just the unique values:

var resultCells = cells.Distinct();

That's really easy. So now, if it's just one point, they'll all be removed except for one. Next, we'll get the outputs at each point. This is, again, pretty easy:

return resultCells.Select(t => items[t.Item1, t.Item2]);

So our whole function is now:

var nrOfRows = items.GetLength(0) / 2.0;
var nrOfColumns = items.GetLength(1) / 2.0;
var fRow = (int)Math.Floor(nrOfRows);
var lRow = (int)Math.Ceiling(nrOfRows);
var fCol = (int)Math.Floor(nrOfColumns);
var lCol = (int)Math.Ceiling(nrOfColumns);

var cells = new List<Tuple<int, int>> {
    new Tuple<int, int>(fRow, fCol),
    new Tuple<int, int>(fRow, lCol),
    new Tuple<int, int>(lRow, fCol),
    new Tuple<int, int>(lRow, lCol)
};
var resultCells = cells.Distinct();
return resultCells.Select(t => items[t.Item1, t.Item2]);

We can also do the whole last part in one pass:

return
    new List<Tuple<int, int>> {
        new Tuple<int, int>(fRow, fCol),
        new Tuple<int, int>(fRow, lCol),
        new Tuple<int, int>(lRow, fCol),
        new Tuple<int, int>(lRow, lCol)
    }
    .Distinct()
    .Select(t => items[t.Item1, t.Item2]);

If you have access to C#7.0 and the ValueTuple NuGet package, the .Select is simpler:

.Select((y, x) => items[y, x])

Now we have eliminated all of our duplication, and we've made it clean, readable, and simple. There's not much to be inferred here, and it's easy to manage.

public static IEnumerable<T> GetCenterItems<T>(this T[,] items)
{
    var nrOfRows = (items.GetLength(0) - 1) / 2.0;
    var nrOfColumns = (items.GetLength(1) - 1) / 2.0;
    var fRow = (int)Math.Floor(nrOfRows);
    var lRow = (int)Math.Ceiling(nrOfRows);
    var fCol = (int)Math.Floor(nrOfColumns);
    var lCol = (int)Math.Ceiling(nrOfColumns);

    return
        new List<Tuple<int, int>> {
            new Tuple<int, int>(fRow, fCol),
            new Tuple<int, int>(fRow, lCol),
            new Tuple<int, int>(lRow, fCol),
            new Tuple<int, int>(lRow, lCol)
        }
        .Distinct()
        .Select(t => items[t.Item1, t.Item2]);
}
\$\endgroup\$
  • 1
    \$\begingroup\$ You can make it even better/shorter by using tuples with named properties, so instead of List<Tuple<int, int>> make it List<(int Row, int Col)> and initlize with just (fRow, fCol) without new Tuple. The query will become cleaner too: things[t.Row, t.Col] \$\endgroup\$ – t3chb0t Apr 26 '18 at 18:56
  • \$\begingroup\$ @t3chb0t Absolutely, feel free to drop that in as an edit if you would like (or alternate answer), I'm unfamiliar with C#7.0 (basically quit using C# as-of-late), so I'm curious to see what that solution looks like, and I'm not the best to write it. :) \$\endgroup\$ – 410_Gone Apr 26 '18 at 18:59
  • \$\begingroup\$ Nice answer. The only thing is nrOfRows suggests a row count, and your changes produce the row midpoint. Thus I would name it rowMidPt and colMidPt to provide a more meaningful (or less confusing) name. \$\endgroup\$ – Rick Davin Apr 30 '18 at 13:22

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.