HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

A class for solving a system of linear equations using Gaussian Elimination

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
solvinglinearequationssystemeliminationforusingclassgaussian

Problem

```
class LinearEquationSolver
{
List rows = new List();
decimal[] solution;

public void AddLinearEquation(decimal result, params decimal[] coefficients)
{
rows.Add(new LinearEquation(result, coefficients));
}

public IList Solve() //Returns a list of coefficients for the variables in the same order they were entered
{
solution = new decimal[rows[0].Coefficients.Count()];

for (int pivotM = 0; pivotM r.Result != 0))
{
LinearEquation row = rows.FirstOrDefault(r => r.NonZeroCount == 1);
if (row == null)
{
break;
}

int solvedIndex = row.IndexOfFirstNonZero;
decimal newSolution = row.Result / row[solvedIndex];

AddToSolution(solvedIndex, newSolution);
}

return solution;
}

private void AddToSolution(int index, decimal value)
{
foreach (LinearEquation row in rows)
{
decimal coefficient = row[index];
row[index] -= coefficient;
row.Result -= coefficient * value;
}

solution[index] = value;
}

private class LinearEquation
{
public decimal[] Coefficients;
public decimal Result;

public LinearEquation(decimal result, params decimal[] coefficients)
{
this.Coefficients = coefficients;
this.Result = result;
}

public decimal this[int i]
{
get { return Coefficients[i]; }
set { Coefficients[i] = value; }
}

public void AddCoefficients(LinearEquation pivotEquation, decimal factor)
{
for (int i = 0; i < this.Coefficients.Count(); i++)
{
this[i] += pivotEquation[i] * factor;
if (Math.Abs(this[i]) < 0.000000001M) //Because sometimes rounding errors mean it's not quite zero, and it needs to be
{

Solution

Enumerable.Count() vs count access

One problem that slows your program's performance is that you call

Coefficients.Count()


In a few places while you are using array instead of an IEnumerable where this is the only way to count the elements and as you can see this is a method rather than being a property. Instead you should simply call Coefficients.Length.

Edit

As @t3chb0t pointed in the comments and discussed here C# will simply return the Count property of the collection even when you call the method .Count() which means Coefficients.Count() & Coefficients.Length are almost equivalent in performance since the method has to do a type checking.

Nevertheless when possible, you should always prefer using your collection's specific functions and properties, rather than the generic ones. In your case Coefficients.Length.

Properties vs fields

Ideally you should always use properties instead of fields

public decimal[] Coefficients;
public decimal Result;


Can become

public decimal[] Coefficients { get; }
public decimal Result { get; set; }


You properties should be as restrictive as possible the less your class exposes the better.

Indexer with public backing collection ?

This doesn't makes much sense. Usually when a class has a indexer it's used internally with a private backing collection, but you have an iterator + a public collection.

In your case I don't see any reason why would you use an indexer here the public array Coefficients is more than enough and it's a lot more readable, so you should remove that indexer and just access the array like this :

public void AddCoefficients(LinearEquation pivotEquation, decimal factor)
{
    for (int i = 0; i < this.Coefficients.Length; i++)
    {
        Coefficients[i] += pivotEquation.Coefficients[i]*factor;
        if (Math.Abs(Coefficients[i]) < 0.000000001M)
            //Because sometimes rounding errors mean it's not quite zero, and it needs to be
        {
            Coefficients[i] = 0;
        }
    }
    this.Result += pivotEquation.Result*factor;
}


Redundant this qualifier

While I do agree that this is up to a personal preference I don't like it. You don't need a single this qualifier in your code it would be better to remove it it's just extra text to read..

Expression bodied properties + LINQ

public int IndexOfFirstNonZero
{
    get
    {
        for (int i = 0; i < Coefficients.Length; i++)
        {
            if (Coefficients[i] != 0)
            {
                return i;
            }
        }
        return -1;
    }
}

public int NonZeroCount
{
    get
    {
        int count = 0;
        for (int i = 0; i < Coefficients.Length; i++)
        {
            if (Coefficients[i] != 0) count++;
        }
        return count;
    }
}


You can really shorten those get only properties using LINQ :

public int IndexOfFirstNonZero
{
    get
    {
        int value = (int) Coefficients.FirstOrDefault(x => x != 0);
        return value == default(int) ? -1 : value;
    }
}

public int NonZeroCount => Coefficients.Count(t => t != 0);

Code Snippets

Coefficients.Count()
public decimal[] Coefficients;
public decimal Result;
public decimal[] Coefficients { get; }
public decimal Result { get; set; }
public void AddCoefficients(LinearEquation pivotEquation, decimal factor)
{
    for (int i = 0; i < this.Coefficients.Length; i++)
    {
        Coefficients[i] += pivotEquation.Coefficients[i]*factor;
        if (Math.Abs(Coefficients[i]) < 0.000000001M)
            //Because sometimes rounding errors mean it's not quite zero, and it needs to be
        {
            Coefficients[i] = 0;
        }
    }
    this.Result += pivotEquation.Result*factor;
}
public int IndexOfFirstNonZero
{
    get
    {
        for (int i = 0; i < Coefficients.Length; i++)
        {
            if (Coefficients[i] != 0)
            {
                return i;
            }
        }
        return -1;
    }
}


public int NonZeroCount
{
    get
    {
        int count = 0;
        for (int i = 0; i < Coefficients.Length; i++)
        {
            if (Coefficients[i] != 0) count++;
        }
        return count;
    }
}

Context

StackExchange Code Review Q#134000, answer score: 3

Revisions (0)

No revisions yet.