2

I'm working on a WPF application, trying to stay strict in separating View, ViewModel and Model.

My application has a few different views in a relatively flat hierarchy. There is one view for editing something we call a substance, and one for classifications, where each substance has a number of classifications.

My question is about the ViewModel classes for the substances and classifications, each is about 1000 lines! The thing is, the code is mostly consisting of get/set properties, with some inherent logic working against my model, as well as some commands and private functions. So I could easily divide the class into a bunch of smaller classes (like one per GroupBox in my view or whatever) with one class owning instances of the other, to get smaller classes.

But this doesn't seem meaningful to me, it seems like solving a symptom, like doing it for its own sake. The size of the class clearly depends on the number of fields related to the substances and classifications, and I can't do anything about that.

Do you think I'm in trouble here? Is it normal for WPF applications to be so dominated, size-wise, by the ViewModels? How serious should I be about limiting class sizes? I could reduce it significantly by changing the way I write braces, but I think it looks more clear now than with Egyptian brackets.

My question isn't really about whether my code is muddy or not (some of it is), but rather if you can actually say that a ViewModel class is muddy just because it's large. To give some perspective, my substance view has 7 lists/datagrids/similar, 13 buttons, 9 comboboxes, 5 checkboxes, 9 textboxes and 3 radio buttons. All in all, that's about 26 rows per control. There are some cases where there's interaction going on between different controls, setting a value in one control will disable and nullify another one etc.

Some code samples:

public RelayCommand AddSubstanceNumberCommand
    {
        get
        {
            return new RelayCommand(
                (x) =>
                {
                    var number = new SubstanceNumber();
                    number.SubstanceID = Id;
                    number.ModifiedBy = Main.op.LoginName;
                    number.ModifiedDate = DateTime.Now;
                    number.CreatedDate = DateTime.Now;
                    number.Type = Converters.NumberTypeToStringConverter.translation.First(t => t.Value == SelectedNewSubstanceNumberType).Key;
                    number.NumberText = Validator.Normalize(NewSubstanceNumber, SelectedNewSubstanceNumberType);
                    var temp = NewSubstanceNumber.Replace("-", "");
                    number.NumberValue = Int32.Parse(temp.Substring(0, temp.Length - 1));
                    if (NewSubstanceNumber != null && !SubstanceNumbers.Any(sn => sn.NumberText.Replace(" ", "") == number.NumberText && sn.Type == number.Type))
                        SubstanceNumbers.Add(number);

                    NewSubstanceNumber = "";
                }, param => this.CanAddSubstanceNumber);
        }
    }



public ObservableCollection<SubstanceGroup> SubstanceGroups
    {
        get
        {
            return Substance.SubstanceGroups;
        }
        set
        {
            Substance.SubstanceGroups = value;
            OnPropertyChanged("SubstanceGroups");
        }
    }



public RelayCommand OpenAllClassificationsCommand
    {
        get
        {
            return new RelayCommand(
            (x) =>
            {
                // Opening tabs for all classifications that don't already have a tab open
                var currentSubstanceTab = Main.SelectedTab;
                var classificationTabsNow = currentSubstanceTab.Tabs.Where(t => t.GetType() == typeof(ClassificationViewModel));
                var toOpen = Classifications.Where(c => !classificationTabsNow.Any(ctn => ctn.Id == c.SubstanceClassificationID));
                toOpen.ToList().ForEach(o => Main.SelectedTab.Tabs.Add(new ClassificationViewModel(Main, o.SubstanceID, o.SubstanceClassificationID, false, new Services.MessageBoxNotifyUserService())));
            });
        }
    }

And here's one that's particularly muddy, and repeated:

public OccupationalExposureLimitUnit OccupationalExposureLimitUnit5
    {
        get
        {
            if (SubstHasOccExpLimits)
                return Substance.OccupationalExposureLimitUnit5;
            else if (GroupForLimits != null)
                return GroupForLimits.OccupationalExposureLimitUnit5;
            return null;
        }
        set
        {
            bool hadValue = SubstHasOccExpLimits;
            if (value.Name_SV == "")
                OccupationalExposureLimitShortTerm2 = null;
            Substance.OccupationalExposureLimitUnit5 = value;

            if (!hadValue && SubstHasOccExpLimits)
                TransferLimitValuesFromGroupToSubstance();
            OnPropertyChanged("OccupationalExposureLimitUnit5");
        }
    }

This property includes some fallback-functionality for when the substance lacks a certain set of values, and some state changes for going to substance-has-values-state when the user starts editing one of the values. There is certainly room for improvement here, but even without it, the class would be a number of times larger than the 200 rows some recommend for class size.

  • You should be extracting small testable pieces of logic to their own classes. That should help quite a bit. – ChaosPandion Jul 28 '14 at 9:05
  • You can use (paid) postsharp.net/model/inotifypropertychanged to avoid explicit notifying logic in your VM properties, which could cut the number of lines significantly. – Den Jul 28 '14 at 9:14
  • So you have data binding and some kind of a data context, which of course must have lots of properties, and accessors for each control, plus logic to interact with the model. I would like to know whether these accessors are highly repetitive so your size is simply NxM, but the structure is simple (no Mud here). Would a sample of the code be feasible? – david.pfx Jul 28 '14 at 10:44
  • My question isn't really about wether my code is muddy or not (some of it is), but rather if you can actually say that a ViewModel class is muddy just because it's large. To give some perspective, my substance view has 7 lists/datagrids/similar, 13 buttons, 9 comboboxes, 5 checkboxes, 9 textboxes and 3 radio buttons. All in all, that's about 26 rows per control. There are some cases where there's interaction going on between different controls, setting a value in one control will disable and nullify another one etc. I'll try to find some good code examples. – Mårten Jul 28 '14 at 11:06
  • If you have to ask, the answer is probably yes. Try to make each layer nested (read up on hierarchical MVC to get some ideas). That should make the problem more manageable. – back2dos Jul 28 '14 at 20:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.