5
\$\begingroup\$

I have seven checkboxes on a data maintenance WinForm, which indicate the days of the week that are "valid" for a particular operation to be performed. These "map" to a list of DayOfWeek enum values on the backing domain object; if the value is present in the list, the box should be checked, otherwise not. Any combination of boxes can be checked. In the "bind" method that sets the UI controls to the proper values of the domain object, here's how those checkboxes are set:

chkValidDayMon.Checked = myModel.ValidDaysOfWeek.Contains(DayOfWeek.Monday);
chkValidDayTue.Checked = myModel.ValidDaysOfWeek.Contains(DayOfWeek.Tuesday);
chkValidDayWed.Checked = myModel.ValidDaysOfWeek.Contains(DayOfWeek.Wednesday);
chkValidDayThu.Checked = myModel.ValidDaysOfWeek.Contains(DayOfWeek.Thursday);
chkValidDayFri.Checked = myModel.ValidDaysOfWeek.Contains(DayOfWeek.Friday);
chkValidDaySat.Checked = myModel.ValidDaysOfWeek.Contains(DayOfWeek.Saturday);
chkValidDaySun.Checked = myModel.ValidDaysOfWeek.Contains(DayOfWeek.Sunday);

The code to read the UI controls' state back out to the domain object is similar; if the box is checked, add the corresponding value to a "clean" (initially empty) list and assign it to the object.

This smells. I was thinking of adding a Dictionary containing the checkboxes and keyed to their corresponding DayOfWeek. But, that sounds like a bit much as well (it certainly wouldn't save many LoC). Any other suggestions?

\$\endgroup\$
  • \$\begingroup\$ @Leonid - While in general I agree with you, there are exactly seven days in a Gregorian week (and in most other Abrahamic culture calendars), so until the entire world adopts the Javanese five-day week I don't foresee this code having to change (not in a way that having a Dictionary would make more difficult, anyway). I could use Linq to create the Dictionary based on the controls, maybe. \$\endgroup\$ – KeithS Aug 22 '12 at 16:37
  • \$\begingroup\$ If you do not want to hide information, then you should go with one of the two approaches that you suggested yourself. Either way you will have to write about 7 lines of code. The dictionary approach is probably better for it cannot be re-factored further. Once the code works, you pretty much know that it will not break. So what that it smells a bit? Seven controls do not justify going overboard, so KISS. The only danger really is programmers who will want to re-factor the working code and may introduce bugs. This is why dictionary is a decent solution. Speed is not an issue at all here. \$\endgroup\$ – Leonid Aug 22 '12 at 18:03
  • \$\begingroup\$ Well, if you make your comment an answer I'll accept it, because the solution I arrived at was to give the checkboxes Tag values for the integer representations of each DayOfWeek, then slurp the Controls (which were already in a GroupBox) into a Dictionary keyed by the Tag. \$\endgroup\$ – KeithS Aug 22 '12 at 19:13
  • \$\begingroup\$ the Tag is of the type object. Why not stick an enum value rather than an integer in there? \$\endgroup\$ – Leonid Aug 22 '12 at 20:03
  • \$\begingroup\$ Because the designer assumes anything you enter into the Tag field is a string, so it actually doesn't really matter either way; I still have to parse it. I could use "Sunday", "Monday" etc as well, it works either way. \$\endgroup\$ – KeithS Aug 22 '12 at 20:27
2
\$\begingroup\$

I'd create an extension method that calculates the day by the name of the checkbox.

I'm using dynamic because I don't know the name of your domain class, just replace it with your name.

Also note, I'm using the web version of CheckBox, substitute ID with whatever version you are using.

Here is the class for the extension:

public static class CheckboxExtensions
{
    public static void DetermineStateFromModel(this CheckBox me, dynamic myModel)
    {
        if (!me.ID.Contains("chkValidDay"))
        {
            // Could throw an exception if required.
            return;
        }

        var dayOfWeek = DayOfWeekFromString(me.ID.Replace("chkValidDay", string.Empty));

        me.Checked = myModel.ValidDaysOfWeek.Contains(dayOfWeek);
    }

    private static DayOfWeek DayOfWeekFromString(string dayOfWeek)
    {
        switch (dayOfWeek)
        {
            case "Mon":
                return DayOfWeek.Monday;
            case "Tue":
                return DayOfWeek.Tuesday;
            // ...
            default:
                throw new ApplicationException("Invalid dayOfWeek specified.");

        }
    }
}

Then your calls would be

chkValidDayMon.DetermineStateFromModel(myModel);
chkValidDayTues.DetermineStateFromModel(myModel);
...

Another thing you might look into is to put the CheckBox objects into a list, then you could just iterate it:

dayCheckBoxList.ForEach(checkBox => checkBox.DetermineStateFromModel(myModel));

Another note, Hungarian casing on variables is not really accepted anymore. It clutters up the code, and there are too many variable types to abbreviate them all to 3 letters. Read this. A better naming convention would be mondayIsValidCheckBox. You would have to modify my extension method to use this new convention.

\$\endgroup\$
  • \$\begingroup\$ String parsing on the name of a control? That smells worse than individual binding/unbinding. \$\endgroup\$ – KeithS Aug 22 '12 at 16:20
  • \$\begingroup\$ I agree that Hungarian casing is bad. I do it on UI controls only, mainly out of old habit (and because changing the type of a control laid out in the Designer requires either ripping it out and replacing it, or touching so much code that changing its name becomes rather trivial) \$\endgroup\$ – KeithS Aug 22 '12 at 16:27
  • 1
    \$\begingroup\$ String parsing on the name of a control falls into a paradigm call Convention over Configuration: en.wikipedia.org/wiki/Convention_over_configuration which I have been using more and more. \$\endgroup\$ – Jeff Vanzella Aug 22 '12 at 17:47
  • 1
    \$\begingroup\$ Its not hiding as long as it is documented and followed. You also have checks in the methods to make sure the convention is being followed, so if somebody doesn't follow it, an exception is thrown. \$\endgroup\$ – Jeff Vanzella Aug 22 '12 at 17:58
  • 2
    \$\begingroup\$ I think it would be better to use Dictionary<string, DayOfWeek> rather than that big case tree. I have not used C# for some time so I am not sure if the hashmap is called Dictionary in .NET :) \$\endgroup\$ – Aleksandar Sep 21 '12 at 17:32

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.