1
\$\begingroup\$

I have a static list of descriptions in a resource and I am filtering down the list based on the index positions of the text in the array

Here is my multi converter:

using OCLMEditor.Data.StudentInfo.Enums;
using System;
using System.Collections.Generic;
using System.Globalization;
using System.Windows.Data;

namespace OCLMEditor.ValueConverters
{
    public class StudentAssignmentTypeFilterConverter : IMultiValueConverter
    {
        public object Convert(object[] values, Type targetType, object parameter, CultureInfo culture)
        {
            StudentAssignmentType[] allTypes = values[0] as StudentAssignmentType[];
            List<StudentAssignmentType> filteredTypes = new List<StudentAssignmentType>();
            int iNumberOfClasses = values[1] != null ? (int)values[1] : 0;

            if (iNumberOfClasses == 3) // Main hall, Class 1 and Class 2
                return allTypes;

            if(iNumberOfClasses == 0)
            {
                // What to return? It should not get here as the combo is disabled if 
                return allTypes;
            }

            for(int i = 0; i < allTypes.Length; i++)
            {
                if(iNumberOfClasses >= 1)
                {
                    if (i == 0 || i == 3 || i == 6 || i == 9 || i == 12 || i == 15 || i == 18)
                        filteredTypes.Add(allTypes[i]);
                }

                if(iNumberOfClasses >= 2)
                {
                    if (i == 1 || i == 4 || i == 7 || i == 10 || i == 13 || i == 16 || i == 19)
                        filteredTypes.Add(allTypes[i]);
                }

                if (iNumberOfClasses == 3)
                {
                    if (i == 2 || i == 5 || i == 8 || i == 11 || i == 14 || i == 17 || i == 20)
                        filteredTypes.Add(allTypes[i]);
                }
            }

            return filteredTypes;
        }

        public object[] ConvertBack(object value, Type[] targetTypes, object parameter, CultureInfo culture)
        {
            throw new NotSupportedException();
        }
    }
}

Do you think the if clauses inside can be simplified somehow?

Specifcally these three if clauses. Eg:

if(iNumberOfClasses >= 1)
{
    if (i == 0 || i == 3 || i == 6 || i == 9 || i == 12 || i == 15 || i == 18)
        filteredTypes.Add(allTypes[i]);
}

I know about using % to get the (modulus(?)) remainder of a division. So if my numbers were 1, 5, 10, 15, 20 I could use number % 5 == 0 to get all of them. But ican't see that I can do that here?

\$\endgroup\$
  • 1
    \$\begingroup\$ How about a better title and a few more details about what the code does etc.? :-] \$\endgroup\$ – t3chb0t Jul 5 '16 at 10:57
  • \$\begingroup\$ @t3chb0t Indeed. But I don't really know how to re-phrase the title. \$\endgroup\$ – Andrew Truckle Jul 5 '16 at 10:58
  • \$\begingroup\$ Well, it's not that hard... you know what it does... make it short and put it in the title... \$\endgroup\$ – t3chb0t Jul 5 '16 at 11:00
  • 1
    \$\begingroup\$ This code doesn't make any sense. There is a condition that reads if (iNumberOfClasses == 3) return allTypes; but the same condition you have later inside the loop... you may remove it there. It will never be reached. \$\endgroup\$ – t3chb0t Jul 5 '16 at 11:14
  • \$\begingroup\$ @t3chb0t Good catch! I don't even need that clause in there. :) \$\endgroup\$ – Andrew Truckle Jul 5 '16 at 11:19
3
\$\begingroup\$

The answer of t3chb0t provides a solution in the right direction. My point of "critique" with that solution is the overdose on LinQ for a simple solution like checking if a collection of numbers contains a certain number.

The point of predefining the collection is a good idea. I would do it like this:

var classesForOne = new [] { 0, 3, 6, 9, 12, 15, 18 };
var classesForTwo = new [] { 1, 4, 7, 10, 13, 16, 19 };
var classesForThree = new [] { 2, 5, 8, 11, 14, 17, 20 };

Now you can check if the variable i is present in a collection. This can be done like this:

if(iNumberOfClasses >= 1)
{
    if (classesForOne.Contains(i))
        filteredTypes.Add(allTypes[i]);
}

Writing it this way, you can combine the two if statements in one simpler statement:

if(iNumberOfClasses >= 1 && classesForOne.Contains(i))
{
    filteredTypes.Add(allTypes[i]);
}

So the whole code with loop would now look like following:

var classesForOne = new [] { 0, 3, 6, 9, 12, 15, 18 };
var classesForTwo = new [] { 1, 4, 7, 10, 13, 16, 19 };
var classesForThree = new [] { 2, 5, 8, 11, 14, 17, 20 };

for(int i = 0; i < allTypes.Length; i++)
{
    if(iNumberOfClasses >= 1 && classesForOne.Contains(i))
        filteredTypes.Add(allTypes[i]);

    if(iNumberOfClasses >= 2 && classesForTwo.Contains(i))
        filteredTypes.Add(allTypes[i]);

    if(iNumberOfClasses == 3 && classesForThree.Contains(i))
        filteredTypes.Add(allTypes[i]);
}

Edit:

The mod implementation:

This edit is not mandatory, it is just provided as extra information to your secondary question about using the modulus. You CAN implement the modulus-way to achieve the same result. I'm not saying this is a better implementation, just showing how it can be done :) . Let's look at each of the three cases.

  • { 0, 3, 6, 9, 12, 15, 18 }

If i is divisible by 3, meaning it has a remainder of 0, it is in the first collection of numbers.

  • { 1, 4, 7, 10, 13, 16, 19 }

If i is divided by 3, remainder 1, it is in the second collection.

  • { 2, 5, 8, 11, 14, 17, 20 }

And for the last, if i is divided by 3, remainder 2, it is in the third.

Of course using this method you're not validating if the number i is valid within the range. If for example i is 22 (remainder is 1), the result would say that it is present in the second collection. You'll have to build in an extra check for this. Here's the implementation:

for(int i = 0; i < allTypes.Length; i++)
{
    if(iNumberOfClasses >= 1 && (i <= 18 && i % 3 == 0))
        filteredTypes.Add(allTypes[i]);

    if(iNumberOfClasses >= 2 && (i <= 19 && i % 3 == 1))
        filteredTypes.Add(allTypes[i]);

    if(iNumberOfClasses == 3 && (i <= 20 && i % 3 == 2))
        filteredTypes.Add(allTypes[i]);
}

In this way, you don't need to predefine a collection, the counterpart is that you have to built an extra check to set the maximum allowed number. Hope this explains it. :)

\$\endgroup\$
  • \$\begingroup\$ Thank you for the extra clarification about Mod. \$\endgroup\$ – Andrew Truckle Jul 5 '16 at 13:16
  • \$\begingroup\$ I prefer the arrays autogenerated. Should you want to add another item you need to update three of them. With automation you just change a single number ;-] even better that they can be created with the mod. \$\endgroup\$ – t3chb0t Jul 5 '16 at 14:57
  • \$\begingroup\$ @t3chb0t Auto-creation certainly has its advantages as you point out, but if the collections never change, it would be overkill. \$\endgroup\$ – Abbas Jul 5 '16 at 15:52
2
\$\begingroup\$

The shortest I can think of is to generate the numbers first and then make the conditions shorter by checking if the i is in the particular collection:

var classesFromZero = Enumerable.Repeat(0, 5)
    .Aggregate(Enumerable.Repeat(0, 1), (arr, next) => arr.Concat(new[] { arr.Last() + 3 }))

var classesFromOne = Enumerable.Repeat(0, 5)
    .Aggregate(Enumerable.Repeat(1, 1), (arr, next) => arr.Concat(new[] { arr.Last() + 3 }))

var classesFromTwo = Enumerable.Repeat(0, 5)
    .Aggregate(Enumerable.Repeat(2, 1), (arr, next) => arr.Concat(new[] { arr.Last() + 3 }))

for (int i = 0; i < allTypes.Length; i++)
{
    if (iNumberOfClasses >= 1 && classesFromZero.Contains(i)) filteredTypes.Add(allTypes[i]);
    if (iNumberOfClasses >= 2 && classesFromOne.Contains(i)) filteredTypes.Add(allTypes[i]);
    if (iNumberOfClasses == 3 && classesFromTwo.Contains(i)) filteredTypes.Add(allTypes[i]);
}

This is just a sample. I'd be good to define the magic numbers as constants.

\$\endgroup\$
  • \$\begingroup\$ I cannot comment on the logic because you didn't tell us what the code exacly does any why does it do what it does ;-] \$\endgroup\$ – t3chb0t Jul 5 '16 at 11:34
  • 1
    \$\begingroup\$ I have a static list of descriptions in a resource and I am filtering down the list based on the index positions of the text in the array. \$\endgroup\$ – Andrew Truckle Jul 5 '16 at 11:39
  • 2
    \$\begingroup\$ @AndrewTruckle Please add this info to the question body. \$\endgroup\$ – Caridorc Jul 5 '16 at 12:08

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.