Krdytkyu

I have an asp.net web application and I want to export to a PDF tables in a particular way.

Tables will have data of type Test. tests have a type(by type i mean an int to tell them apart) and for every type i'm creating a TestTable and for every Test a column in each TestTable , there are 9 types. That's why there are 9 TestTables but in tests there might be for example only two different types.

Is there a better way to do this without creating two arrays that their might be useless

public IEnumerable<TestTable> TestHistoryItems(GetTestExportRequest request)

{



    IEnumerable<Test> tests = repository.FindBy(t => t.Visit.pid == request.PetId && 

                                                                t.Visit.dateofvisit >= request.Start && t.Visit.dateofvisit <= request.End).OrderByDescending(t => t.stamp);



    TestTable testTables = new TestTable[9] // all the TestTables are 9 but it might need less depending on tests

    {                                         //every TestTable needs tests, of type IEnumerable<Test>.

        new TestTable(),

        new TestTable(),

        new TestTable(),

        new TestTable(),

        new TestTable(),

        new TestTable(),

        new TestTable(),

        new TestTable(),

        new TestTable()

    };



    List<Test> testTypes = new List<Test>[9] 

    {

        new List<Test>(),

        new List<Test>(),

        new List<Test>(),

        new List<Test>(),

        new List<Test>(),

        new List<Test>(),

        new List<Test>(),

        new List<Test>(),

        new List<Test>()

    };



    foreach (Test test in tests) 

    {

        switch (test.type)

        {

            case 3: GetTable(test, ref testTables[0], 3); testTypes[0].Add(test); break; 

            case 4: GetTable(test, ref testTables[1], 4); testTypes[1].Add(test); break;

            case 5: GetTable(test, ref testTables[2], 5); testTypes[2].Add(test); break;

            case 6: GetTable(test, ref testTables[3], 6); testTypes[3].Add(test); break;

            case 7: GetTable(test, ref testTables[4], 7); testTypes[4].Add(test); break;

            case 8: GetTable(test, ref testTables[5], 8); testTypes[5].Add(test); break;

            case 9: GetTable(test, ref testTables[6], 9); testTypes[6].Add(test); break;

            case 10: GetTable(test, ref testTables[7], 10); testTypes[7].Add(test); break;

            case 16: GetTable(test, ref testTables[8], 16); testTypes[8].Add(test); break;

         }

    }    

    int i = 0;

    foreach (TestTable test in testTables)

    {

        testTables[i].Tests = testTypes[i]; //add tests in testTables

        i++;

    }



    return testTables;

}



public void GetTable(Test test, ref TestTable table, int index)

{

    if (table.Type != index) 

    {

        table.TestName = test.TestTypeName; //One Time for every TestTable

        table.Type = index; //One Time for every TestTable

        table.Rows = test.TestDatas.Count(); //One Time for every TestTable; Rows of the table

    }

    table.Columns++; // columns are the number of tests in every TestTable

}

Here is the Class TestTable.

public class TestTable

{

    public string TestName { get; set; }

    public int Type { get; set; }

    public IEnumerable<Test> Tests { get; set; }

    public int Rows { get; set; }

    public int Columns { get; set; }

}

The program works as expected but I think this is wrong. Can't think of a better way to do this, I'm new to coding.

How can I make this code better?

Some quick remarks:

• pid, dateofvisit, stamp, type are all properties and thus should follow Microsoft's naming guidelines. Moreover "pid" is fairly meaningless, why not call this "PetId" as well?




• testTypes does not convey its actual contents.




• Using an int to indicate a type is meaningless. I don't know what a test of type "1" is, or a "2". You should translate this into an enum with meaningful values.




• WRT // columns are the number of tests in every TestTable: why do you need this? Can't you just count the entries in the property Tests?




• Why is GetTable(Test test, ref TestTable table, int index) a public method? I doubt this is re-used elsewhere. Please apply the correct access modifier to a method, class, property, field etc.

I find your logic hard to understand. I've tried to rewrite it, based on what I think is happening, and this is what I came up with:

    public IEnumerable<TestTable> TestHistoryItems()

    {

        IEnumerable<Test> tests = // get data



        var testTables = new List<TestTable>();



        foreach (var testType in new int { 3, 4, 5, 6, 7, 8, 9, 10, 16 })

        {

            var relevantTests = tests.Where(x => x.Type == testType).ToList();



            if (!relevantTests.Any())

            {

                continue;

            }



            testTables.Add(GetTestTable(relevantTests));

        }



        return testTables;

    }



    private TestTable GetTestTable(IReadOnlyCollection<Test> tests)

    {

        var sampleTest = tests.First();

        return new TestTable

        {

            TestName = sampleTest.TestTypeName,

            Type = sampleTest.Type,

            Rows = sampleTest.TestDatas.Count,

            Tests = tests

        };

    }

You could even reduce the first method even more:

    public IEnumerable<TestTable> TestHistoryItems()

    {

        IEnumerable<Test> tests = // get data



        return new { 3, 4, 5, 6, 7, 8, 9, 10, 16 }

            .Select(testType => tests.Where(x => x.Type == testType).ToList())

            .Where(relevantTests => relevantTests.Any())

            .Select(GetTestTable)

            .ToList();

    }

To me, this looks a lot clearer: I see there's a list of test types you filter on, and based on that you make a List of TestTables for each type that has actual tests.

Again: I'm not 100% sure this is what you're doing, and that's because of the convoluted way your code works. And that is a major problem: good code can almost be "read" as a story. When I need to spend time trying to figure out that index is actually a test type that you've re-purposed, etc., that's a waste of time and energy.

A few observations, in addition to what BCdotWEB already pointed out:

• There's a fair bit of repetition in that switch statement. You're using a type ID to look up a specific TestTable instance, so using a Dictionary instead of a List would be more appropriate here.


• Using multiple collections, where objects are related to each other by index, is fairly brittle. Storing them together is safer. Changing TestTable.Tests to a List<Test> would help, but that may not be desirable because it allows outside modifications.


• All TestTable properties have public setters. That's often a bad idea, as it allows any code to modify them, which can put a TestTable instance in an invalid state. Use a constructor to ensure that an instance is created in a valid state (any required data should be passed in via arguments), and don't make properties writable if they don't need to be.


• 
  TestTable is a reference type, and TestTable does not reassign table, so there's no need to pass it by ref.


• There's a lot of 'magic numbers' in the code - numbers whose meaning is unclear. Also note that hard-coded type IDs means you'll have to modify your code whenever a new type ID needs to be added. That's not ideal.

All this makes the code fairly difficult to understand, but it looks like you're simply grouping tests by their type ID, where a TestTable represents a collection of related tests.

If that's the case, then your code can be simplified to the following:

var testTables = new Dictionary<int, TestTable>();

foreach (var test in tests)

{

    if (!testTables.TryGetValue(test.type, out var testTable))

    {

        // This test is the first of its kind, so create a table for it:

        testTable = new TestTable {

            TestName = test.TestTypeName,

            Type = test.type,

            Rows = test.TestDatas.Count()

        };

        testTables[test.type] = testTable;

    }



    // Add the test to the table (this relies on Tests being a List):

    testTable.Tests.Add(test);

    testTable.Columns += 1;

}

return testTables.Values.ToArray();

Or, if you're familiar with Linq:

return tests

    .GroupBy(test => test.type)

    .Select(testsGroup =>

    {

        // Pick the first test from the group to determine the name and row count:

        var test = testsGroup.First();



        // NOTE: I'd recommend using a proper constructor here instead:

        return new TestTable {

            TestName = test.TestTypeName,

            Type = test.type,

            Rows = test.TestDatas.Count(),

            Columns = testsGroup.Count(),

            Tests = testsGroup.ToList(), // Tests does not need to be a List here

        };

    })

    .ToArray();

'GroupBy' is actually an accurate description of what you're doing here - something that was not very clear in the original code.

Note that these alternatives do not necessarily return results in the same order. If that's important to you then just add an OrderBy(table => table.Type) call before the final ToArray() call. Also note that the result only contains TestTable instances for those groups that actually contain any tests. Then again, without tests you can't determine the name of a group, so I'd consider that an improvement.