4
\$\begingroup\$

I'm working on a small command line application in c# where data is read from a CSV file along with a query and KNN is calculated for the query based on the data.

My problem isn't the algorithm at all, I understand it. Im having difficulty working with my code though. I think it's down to bad planning.

Here's my object for holding a "row" of data. the data is based on weather data and if something was played that day or not.

class dataRow
{
    public double sun {get; set;} 
    public double temp {get; set;} 
    public double humidity {get; set;} 
    public double wind {get; set;}
    public bool play {get; set;}

    public override string ToString()
    {
        return string.Format("\n\tSun: {0}\n\tTemp: {1}\n\tHumidity: {2}\n\tWind: {3}\n\tPlay: {4}", sun, temp, humidity, wind, play);
    }

    public double[] toArray()
    {
        return new double[] { sun, temp, humidity, wind };
    }    
}

I'm trying to Znormalize the data and the query before I work out euclidean distance between points but I'm stuck in a rut of annoying nested loops and I have a hunch there's a better way to code what I'm doing

class KNN
{
    public int numK;
    List<dataRow> dataSet;
    dataRow QueryRow;
    dataRow stdDev;
    dataRow avg;

    public KNN(String[] input)
    {
        numK = Int32.Parse(input[0]);
        dataSet = dataImport.import(input[1]);
        //inputRow = dataImport.import(input[2])[0];
        avg = getAverage(toDoubleList(dataSet));
    }

    public List<double[]> toDoubleList(List<dataRow> input)
    {
        var doubleList = new List<double[]>();         

        for(int i =0; i<input.Count; i++)
        {
            doubleList[i] = input[i].toArray();
        }

        return doubleList;
    }

    public dataRow getAverage(List<double[]> input)
    {
        var avgTotals = new double[4];
        var avgRow = new dataRow();

        for (int y = 0; y < 3; y++)
        {
            double avg = 0;

            for (int x = 0; x < input.Count; x++)
            {
                avg += input[x][y];
            }

            avgTotals[y] = avg / input.Count;
        }

        avgRow.sun = avgTotals[0];
        avgRow.temp = avgTotals[1];
        avgRow.sun = avgTotals[2];
        avgRow.wind = avgTotals[3];

        return avgRow;         
    }

    public dataRow getStdDev(List<double[]> input)
    {
        var stdDevTotals = new double[4];
        var stdDevRow = new dataRow();

        for (int y = 0; y < 3; y++)
        {
            double stdDev = 0;

            for (int x = 0; x < input.Count; x++)
            {
                stdDev += Math.Pow(input[x][y] - this.avg.toArray()[y], 2);
            }

            stdDev = Math.Sqrt(stdDev/input.Count);
            stdDevTotals[y] = stdDev / input.Count;
        }

        stdDevRow.sun = stdDevTotals[0];
        stdDevRow.temp = stdDevTotals[1];
        stdDevRow.sun = stdDevTotals[2];
        stdDevRow.wind = stdDevTotals[3];

        return stdDevRow;    
    }
}

Any advice or ideas? (All harsh criticism welcome!)

\$\endgroup\$
  • 1
    \$\begingroup\$ To get you right, your code is basically working? \$\endgroup\$ – kleinfreund Feb 7 '14 at 14:02
  • \$\begingroup\$ sounds to me like it works @kleinfreund. the first set of wording was weird. but it sounds like it works when read all the way through \$\endgroup\$ – Malachi Feb 7 '14 at 14:40
  • 2
    \$\begingroup\$ Method, Property, and Class names should be PascalCase in C#. \$\endgroup\$ – Magus Feb 7 '14 at 17:44
  • \$\begingroup\$ Yes, I'm basically pulling in CSV data, creating a list of the datRow objects then having to pull data from each objects and insert them into arrays so I can loop through them and do calculations (trying to get avg, stdDev and then the Znormalization) Pardon the poor phrasing on the question. What I want to know is if there's a better or easier way to do what I'm doing? @Magus I originally learned Java, old habits die hard. Noted though, thank you. \$\endgroup\$ – Eogcloud Feb 7 '14 at 18:40
  • 1
    \$\begingroup\$ As pointed out the class names are not too good. Public members (including properties) stay PascalCase. Methods are PascalCase at all times (no excuses). Avoid abusing the var keyword, it's mainly handy for extremely long names. Your for loop variables should be named i, j, k, l ... rather than x and y. Also don't bother giving your variables short names, as with lots of similar short names it becomes unreadable. Why is numK public? Also what is KNN??? Try and use good/sensible names if you are unsure, put a comment saying //Need to think of good name here. My rant is over. \$\endgroup\$ – Ali Caglayan Feb 18 '14 at 14:58
2
\$\begingroup\$
  1. As others mentioned in comments, you should follow the .Net naming guidelines. It will make your code consistent with all other .Net code.

  2. Try to avoid abbreviated names like KNN, because they can be confusing. And if that's supposed to mean k-nearest neighbors, then I don't see how does your class compute that.

    This applies also to names like numK.

  3. Computing some statistics on your data should be separate from parsing inputs. So the constructor of KNN should take directly numK and dataSet and it should be the job of its caller to parse the inputs.

  4. If the common approach is to create a DataRow once and then never change it again, consider making it immutable: make the property setters private and create a constructor that can be used to set them.

    You could also add a constructor from an array of doubles, as an counterpart of ToArray().

  5. Both your GetAverage() and GetStdDev() follow the same pattern. I would try to abstract away this pattern into a separate method, to avoid repetition:

    public static DataRow AggregateRows(
        this IList<DataRow> rows, Func<IEnumerable<double>, double> func)
    {
        return new DataRow
        {
            Sun = func(rows.Select(row => row.Sun)),
            Temp = func(rows.Select(row => row.Temp)),
            Humidity = func(rows.Select(row => row.Humidity)),
            Wind = func(rows.Select(row => row.Wind))
        };
    }
    

    With this, both methods can be significantly simplified using the LINQ method Average():

    public DataRow Average()
    {
        return dataSet.AggregateRows(values => values.Average());
    }
    
    public DataRow StdDev()
    {
        return dataSet.AggregateRows(
            values =>
            {
                var average = values.Average();
                var variance = values.Average(value => Math.Pow(value - average, 2));
                return Math.Sqrt(variance);
            });
    }
    

    Note that the structure of the expression for computing variance is very similar to the way it's normally written: E[(X - E[X])2].

    Also, if you do it this way, you won't need the ToArray() method anymore.

  6. If you have many types whose ToString() follows the same pattern, you could implement it just once using reflection and then use that implementation everywhere.

\$\endgroup\$

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.