Narek
Narek

Reputation: 39871

Can I make a generic function out of these two?

Here are two functions (class methods):

private static Vector2[] ConvertVector2(Vector2[] values)
{
    var maxX = float.MinValue;
    var maxY = float.MinValue;
    var minX = float.MaxValue;
    var minY = float.MaxValue;

    foreach (var point in values)
    {
        maxX = Mathf.Max(point.x, maxX);
        minX = Mathf.Min(point.x, minX);
        maxY = Mathf.Max(point.y, maxY);
        minY = Mathf.Min(point.y, minY);
    }

    var result = new Vector2 [4]
    {
        new Vector2(minX, minY),
        new Vector2(maxX, maxY),
        new Vector2(maxX, minY),
        new Vector2(minX, maxY),
    };

    return result;
}


private static Vector3[] ConvertVector3(Vector3[] values)
{
    var zValue = values[0].z;
    var maxX = float.MinValue;
    var maxY = float.MinValue;
    var minX = float.MaxValue;
    var minY = float.MaxValue;

    foreach (var point in values)
    {
        maxX = Mathf.Max(point.x, maxX);
        minX = Mathf.Min(point.x, minX);
        maxY = Mathf.Max(point.y, maxY);
        minY = Mathf.Min(point.y, minY);
    }

    var result = new Vector3 [4]
    {
        new Vector3(minX, minY, zValue),
        new Vector3(maxX, maxY, zValue),
        new Vector3(maxX, minY, zValue),
        new Vector3(minX, maxY, zValue),
    };

    return result;
}

where Vector2 and Vector3 are structs defined in Unity game engine. I am new to C# and I am trying to figure out if I can use generic functions to not duplicate tons of code in these 2 functions.

Upvotes: 0

Views: 170

Answers (3)

Piotr Kula
Piotr Kula

Reputation: 9821

A more elegant way would be to use Extensions instead.

public static class VectorExtensions
{
    public static Vector2[] ToMaxVector(this Vector2[] values)
    {
        var maxX = float.MinValue;
        var maxY = float.MinValue;
        var minX = float.MaxValue;
        var minY = float.MaxValue;

        foreach (var point in values)
        {
            maxX = Mathf.Max(point.x, maxX);
            minX = Mathf.Min(point.x, minX);
            maxY = Mathf.Max(point.y, maxY);
            minY = Mathf.Min(point.y, minY);
        }

        var result = new Vector2[4]{
            new Vector2(minX, minY), 
            new Vector2(maxX, maxY), 
            new Vector2(maxX, minY), 
            new Vector2(minX, maxY) 
        };

        return result;
    }
}

This way you reduce implementation friction because it gets added to intellisense making for easier implementation.

It still looks like duplicated code but it's code for two different things. Testability of this is also a lot easier since you are type safe. With generics you have a lot of unsafe type issues.

Then you could just use it as follows.

 var maxValues = vectors.ToMaxVector();
  • where vectors is a an array of vectors. "Vector2[]"

Upvotes: -2

Panagiotis Kanavos
Panagiotis Kanavos

Reputation: 131334

Short version

You can reduce the functions to :

private static Vector2[] ConvertVector2(Vector2[] values)
{
    var initial = new Stats2(float.MinValue, float.MaxValue, float.MinValue, float.MaxValue);

    return values.Aggregate(initial,Stats2.Apply,
        acc => new [] {
            new Vector2(acc.MinX, acc.MinY),
            new Vector2(acc.MaxX, acc.MaxY),
            new Vector2(acc.MaxX, acc.MinY),
            new Vector2(acc.MinX, acc.MaxY),
        });
}

by treating the comparison and result generation code as functions and passing them as parameters to a folding function. This common pattern is implemented by LINQ through the Enumarable.Aggregate method

Long version

You can use LINQ's Enumerable.Aggregate to calculate custom aggregations on top of any IEnumerable sequence. Aggregate applies a function to every element in a sequence whose arguments are the current element and the result of the previous operation. Another way to view it is performing an operation inside a loop between the current element and a stored result. That's what your current code does already.

Min and Max are specializations whose aggregation function is Min and Max respectively.

The following snippet uses the Aggregate overload that accepts an aggregation function and a final result selector to produce the result array.

Due to laziness I used the built-in Vector2 class but added a Stats2 class to make the selector cleaner.

struct Stats2
{
    public readonly float MaxX;
    public readonly float MinX;
    public readonly float MaxY;
    public readonly float MinY;

    public Stats2(float maxX, float minX, float maxY, float minY)
        => (MaxX, MinX, MaxY, MinY) = (maxX, minX, maxY, minY);
}

var values = new [] 
{
    new Vector2(1,1),
    new Vector2(2,2),
    new Vector2(3,3),
    new Vector2(4,1),
};

ConnvertVector2 can be written like this :

private static Vector2[] ConvertVector2(Vector2[] values)
{
    var initial = new Stats2(float.MinValue, float.MaxValue, float.MinValue, float.MaxValue);

    return values.Aggregate(initial,
        (acc, point) => new Stats2(
                Math.Max(point.X, acc.MaxX),
                Math.Min(point.X, acc.MinX),
                Math.Max(point.Y, acc.MaxY),
                Math.Min(point.Y, acc.MinY)),
        acc => new [] {
                new Vector2(acc.MinX, acc.MinY),
                new Vector2(acc.MaxX, acc.MaxY),
                new Vector2(acc.MaxX, acc.MinY),
               new Vector2(acc.MinX, acc.MaxY),
        });
}

ConvertVector3 performs the same calculation with a small difference in the result selector, which uses the Z value of the first element:

    private static Vector3[] ConvertVector3(Vector3[] values)
    {
        var zValue = values[0].Z;
        var initial = new Stats2(float.MinValue, float.MaxValue, float.MinValue, float.MaxValue);
        return values.Aggregate(initial,
            (acc, point) => new Stats2(
                    Math.Max(point.X, acc.MaxX),
                    Math.Min(point.X, acc.MinX),
                    Math.Max(point.Y, acc.MaxY),
                    Math.Min(point.Y, acc.MinY)),
            acc => new [] {
                  new Vector3(acc.MinX, acc.MinY,zValue),
                  new Vector3(acc.MaxX, acc.MaxY,zValue),
                  new Vector3(acc.MaxX, acc.MinY,zValue),
                  new Vector3(acc.MinX, acc.MaxY,zValue),
            });
    }

One improvement could be to move the aggregate functions to Stats2 itself :

struct Stats2
{
    public readonly float MaxX;
    public readonly float MinX;
    public readonly float MaxY;
    public readonly float MinY;

    public Stats2(float maxX, float minX, float maxY, float minY)
        => (MaxX, MinX, MaxY, MinY) = (maxX, minX, maxY, minY);

    public static Stats2 Apply(Stats2 acc,Vector2 point) =>
        new Stats2(  Math.Max(point.X, acc.MaxX),
                     Math.Min(point.X, acc.MinX),
                     Math.Max(point.Y, acc.MaxY),
                     Math.Min(point.Y, acc.MinY));

    public static Stats2 Apply(Stats2 acc,Vector3 point) =>
        new Stats2(  Math.Max(point.X, acc.MaxX),
                     Math.Min(point.X, acc.MinX),
                     Math.Max(point.Y, acc.MaxY),
                     Math.Min(point.Y, acc.MinY));
}

ConvertVector2 can be reduced to:

private static Vector2[] ConvertVector2(Vector2[] values)
{
    var initial = new Stats2(float.MinValue, float.MaxValue, float.MinValue, float.MaxValue);

    return values.Aggregate(initial,Stats2.Apply,
        acc => new [] {
            new Vector2(acc.MinX, acc.MinY),
            new Vector2(acc.MaxX, acc.MaxY),
            new Vector2(acc.MaxX, acc.MinY),
            new Vector2(acc.MinX, acc.MaxY),
        });
}

Upvotes: 2

jnovo
jnovo

Reputation: 5769

I don't think it makes sense to use generics since there is no useful common base class or interface implemented by both Vector2 and Vector3.

Given that the only difference between both functions is that the 3D version sets a fixed Z value for all the result vectors I'd simply keep the logic in the 2D function and then re-use it from the 3D one setting the Z value, for instance:

private static Vector2[] ConvertVector2(Vector2[] values)
{
    var maxX = float.MinValue;
    var maxY = float.MinValue;
    var minX = float.MaxValue;
    var minY = float.MaxValue;

    foreach (var point in values)
    {
        maxX = Mathf.Max(point.x, maxX);
        minX = Mathf.Min(point.x, minX);
        maxY = Mathf.Max(point.y, maxY);
        minY = Mathf.Min(point.y, minY);
    }

    var result = new Vector2 [4]
    {
        new Vector2(minX, minY),
        new Vector2(maxX, maxY),
        new Vector2(maxX, minY),
        new Vector2(minX, maxY),
    };

    return result;
}

private static Vector3[] ConvertVector3(Vector3[] values) =>
   ConvertVector2(
      values.Select(v => new Vector2(v.X, v.Y)).ToArray()) // Convert to 2D
   .Select(v => new Vector3(v.X, v.Y, values[0].Z))
   .ToArray();

As a side note, you can simplify the ConvertVector2 function; for instance:

private static Vector2[] ConvertVector2(Vector2[] values)
{
    var xs = values.Select(v => v.X); // .ToArray() for efficiency
    var ys = values.Select(v => v.Y); // .ToArray() for efficiency
    var min = new Vector2(xs.Min(), ys.Min());
    var max = new Vector2(xs.Max(), ys.Max());
    // If your Vector2 implements IComparable, use instead:
    // var min = values.Min();
    // var max = values.Max();

    return new[]
    {
        new Vector2(min.X, min.Y),
        new Vector2(max.X, max.Y),
        new Vector2(max.X, min.Y),
        new Vector2(min.X, max.Y),
    };
}

Please note that I'm using System.Numerics.Vector* here, adjust to your Unity classes as needed (eg. lowercase X and Y properties).

Upvotes: 1

Related Questions