3
\$\begingroup\$

I have a dropdownbox, ActiviteitAardItems, where ActiviteitAard items can be checked (checkbox). If one (or more) are checked the property Opacity will be changed, and the code beneath will be executed. The Tijdblok mentioned below also has a property Activiteit and ActiviteitAard.

public double Opacity
{
    get
    {
        if (Planning.ActiviteitAardItems.Where(aa => aa.IsChecked == true).Count() > 0)
        {
            if (this.Tijdblokken.Where(t => t.Activiteit != null && t.Activiteit.ActiviteitAard != null).Count() > 0)
            {
                Tijdblok tijdblok;
                var tijdblokken = new List<Tijdblok>();

                foreach (var item in Planning.ActiviteitAardItems.Where(aa => aa.IsChecked == true))
                {
                    tijdblok = this.Tijdblokken.Where(t => t.Activiteit != null && t.Activiteit.ActiviteitAard != null && t.Activiteit.ActiviteitAard.Code == item.Code).FirstOrDefault();
                    if (tijdblok != null)
                        tijdblokken.Add(tijdblok);
                }

                tijdblok = tijdblokken.OrderByDescending(tb => tb.Activiteit.RoosterKleurPrioriteit).FirstOrDefault();

                if (tijdblok != null && tijdblok.Activiteit != null && tijdblok.Activiteit.ActiviteitAard != null)
                {
                    if (Planning.ActiviteitAardItems.Where(aa => aa.IsChecked).Select(x => x.Code).Contains(tijdblok.Activiteit.ActiviteitAard.Code))
                    {
                        return 1.0;
                    }
                    else
                    {
                        return 0.1;
                    }
                }
                else
                {
                    return 0.1;
                }

            }
            else
            {
                return 0.1;
            }
        }

        return 1.0;
    }
}
\$\endgroup\$
1
  • 3
    \$\begingroup\$ I think a getter that does that much, deserves to be a full-fledged method. \$\endgroup\$ Apr 10, 2013 at 1:45

1 Answer 1

4
\$\begingroup\$

I think there is probably more that could be offered as the code like something that could do with a makeover, but here's a couple of minor points for consideration.

  1. To avoid any potential full enumeration of the items to retrieve the count of items in the list use either the .Count property or the option I prefer .Any(). e.g. Planning.ActiviteitAardItems.Where(aa => aa.IsChecked == true).Any()

  2. I personally think lines like aa.Checked == true is unecessary noise. You can achieve the same result by just going .Where(aa => aa.IsChecked).

  3. If you are using the same IEnumerable multiple times consider first assigning it to a local variable and doing a ToList() on it. This means that you will avoid any potential multiple enumerations of the dataset as ToList() will force it all back into memory.

    For example

    var ardCheckedItems = Planning.ActiviteitAardItems.Where(aa => aa.IsChecked).ToList();
    

    You can now use this same variable in a couple of places in your code.

  4. Does 1.0 and 0.1 mean anything? You could perhaps convert them to constants to avoid any repetition. See example below.

  5. Here is an attempt to reduce the Arrow code effect we sometimes find ourselves writing and seems evident in this case.

public double Opacity
{
    get
    {
        const double returnOne = 1.0; // for lack of better names
        const double returnTwo = 0.1;

        var ardCheckedItems = Planning.ActiviteitAardItems.Where(aa => aa.IsChecked).ToList();

        if(!ardCheckedItems.Any()) return returnOne;            

        tijdBlokkenItems = this.Tijdblokken.Where(t => t.Activiteit != null && t.Activiteit.ActiviteitAard != null).ToList();

        if(!tijdBlokkenItems.Any()) return returnTwo;

        Tijdblok tijdblok;
        var tijdblokken = new List<Tijdblok>();

        // See Edit below for an example of how to possibly re-write this
        foreach (var item in ardCheckedItems)
        {
            tijdblok = tijdBlokkenItems.Where(t => t.Activiteit.ActiviteitAard.Code == item.Code).FirstOrDefault();
            if (tijdblok != null)
                tijdblokken.Add(tijdblok);
        }

        tijdblok = tijdblokken.OrderByDescending(tb => tb.Activiteit.RoosterKleurPrioriteit).FirstOrDefault();

        var objectsNotNull = tijdblok != null && tijdblok.Activiteit != null && tijdblok.Activiteit.ActiviteitAard != null;
        var anyCheckedItems = ardCheckedItems.Any(x => x.Code == tijdblok.Activiteit.ActiviteitAard.Code);

        return objectsNotNull && anyCheckedItems ? returnOne : returnTwo;
    }
}

EDIT: After Trevor pointed me in the right direction in order to improve the foreach statement I did a quick search on if it would be easy enough to do an extension method to return items that are not Null. Stack overflow to the rescue https://stackoverflow.com/questions/14469159/linq-selectmany-and-where-extension-method-ignoring-nulls. So altering that slighty I came up with and extension method like so:

public static IEnumerable<TResult> WhereNotNull<TSource, TResult>(
            this IEnumerable<TSource> source, Func<TSource, TResult> selector)
            where TResult : class
{
    return source.Select(selector).Where(sequence => sequence != null);
}

which means I believe we can re-write the foreach like so:

var tijdblokken = ardCheckedItems.WhereNotNull(ard => tijdBlokkenItems.FirstOrDefault(t.Activiteit.ActiviteitAard.Code == ard.Code)).ToList();
\$\endgroup\$
7
  • \$\begingroup\$ Thanks for your comment, some nice 'tips' in there :) I am actually wondering about the part you also commented on. I want to know if there is an easier (cleaner) way of checking if an ActiviteitAard is also checked in the list. \$\endgroup\$
    – frG
    Apr 8, 2013 at 9:58
  • \$\begingroup\$ The code is inside a get method of a property, and is responsible for calculating the opacity value based on the checked items. \$\endgroup\$
    – frG
    Apr 8, 2013 at 10:13
  • \$\begingroup\$ @dreza - something like this: var tijdblokken = ardCheckedItems.SelectMany(ard => tijdBlokkenItems.Where(t => t.Activiteit.ActiviteitAard.Code == ard.Code).FirstOrDefault()).Where(t => t != null).ToList();? \$\endgroup\$ Apr 8, 2013 at 11:50
  • \$\begingroup\$ @TrevorPilley only logical occasion where FirstOrDefaults can be chained are when we know that underlying IEnumerable cannot have more than one element. If @frG can confirm that t.Activiteit.ActiviteitAard.Code == item.Code cannot be true for more than one t (which is reasonable), then your snippet could be rewritten var tijdblokken = ardCheckedItems.SelectMany(ard => tijdBlokkenItems.Where(t => t.Activiteit.ActiviteitAard.Code == ard.Code)).ToList(); \$\endgroup\$ Apr 8, 2013 at 13:36
  • \$\begingroup\$ @TrevorPilley I think you should not have the firstOrDefault() in the the SelectMany, anyways. Am i missing something? I can't try it right now. \$\endgroup\$ Apr 8, 2013 at 13:41

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.