C# checkboxes - Can I clean this up?

Question

Hi all...

This function is called from the form_onload. I am basically reading the registry, determining which checkboxes are checkmarked, and then reflecting that in the GUI.

Is there any way to condense this and write better code? How about using CheckState property?

Thanks.

Woody

private void checkExcelSettings()
    {
        // Read what the values are for the checkboxes first and assign them to a string.
        string _excelEnable = Convert.ToString(Registry.GetValue(@"HKEY_CURRENT_USER\Software\Mask Data\", "ExcelEnableHash", "Unchecked"));
        string _excelSSN = Convert.ToString(Registry.GetValue(@"HKEY_CURRENT_USER\Software\Mask Data\", "ExcelSSNHash", "Unchecked"));
        string _excelCC = Convert.ToString(Registry.GetValue(@"HKEY_CURRENT_USER\Software\Mask Data\", "ExcelCCHash", "Unchecked"));
        string _excelWells = Convert.ToString(Registry.GetValue(@"HKEY_CURRENT_USER\Software\Mask Data\", "ExcelWellsHash", "Unchecked"));
        string s=@"t\""; //unimportant no-op to placate stackoverflow syntax highlighter.


        // Now let's make sure we reflect what they are supposed to be in the GUI.
        if (_excelEnable == "Checked")
        {
            chkbxExcelEnable.Checked = true;
        }
        else
        {
            chkbxExcelEnable.Checked = false;
        }

        if (_excelSSN == "Checked")
        {
            chkbxExcelSSN.Checked = true;
        }
        else
        {
            chkbxExcelSSN.Checked = false;
        }

        if (_excelCC == "Checked")
        {
            chkbxExcelCC.Checked = true;
        }
        else
        {
            chkbxExcelCC.Checked = false;
        }

        if (_excelWells == "Checked")
        {
            chkbxExcelWellsFargo.Checked = true;
        }
        else
        {
            chkbxExcelWellsFargo.Checked = false;
        }
    }

Answer 1

Well, you could at least narrow it down to:

chkbxExcelCC.Checked = _excelCC.Equals("Checked");

This way you avoid all the if/else statements.

Answer 2

You can remove all the unnecessary if/else conditions by placing the condition inline with the assignment. You can also make the main key path a constant. However, to really simplify it down, you can take the repeated logic of looking up a key and comparing against "Checked" and put it into a separate method:

private void checkExcelSettings()
{
    // Now let's make sure we reflect what they are supposed to be in the GUI.
    chkbxExcelEnable.Checked = IsChecked("ExcelEnableHash");
    chkbxExcelSSN.Checked = IsChecked("ExcelSSNHash");
    chkbxExcelCC.Checked = IsChecked("ExcelCCHash");
    chkbxExcelWellsFargo.Checked = IsChecked("ExcelWellsHash");
}

private static bool IsChecked(string regValue)
{
    return Convert.ToString(
               Registry.GetValue(
                   @"HKEY_CURRENT_USER\Software\Mask Data\",
                   regValue,
                   "Unchecked")) == "Checked";
}

Answer 3

private void checkExcelSettings()
{
    // Read what the values are for the checkboxes first and assign them to a string.
    string _excelEnable = Convert.ToString(Registry.GetValue(@"HKEY_CURRENT_USER\Software\Mask Data\", "ExcelEnableHash", "Unchecked"));
    string _excelSSN = Convert.ToString(Registry.GetValue(@"HKEY_CURRENT_USER\Software\Mask Data\", "ExcelSSNHash", "Unchecked"));
    string _excelCC = Convert.ToString(Registry.GetValue(@"HKEY_CURRENT_USER\Software\Mask Data\", "ExcelCCHash", "Unchecked"));
    string _excelWells = Convert.ToString(Registry.GetValue(@"HKEY_CURRENT_USER\Software\Mask Data\", "ExcelWellsHash", "Unchecked"));

    // Now let's make sure we reflect what they are supposed to be in the GUI.
    chkbxExcelEnable.Checked = (_excelEnable == "Checked");
    chkbxExcelSSN.Checked = (_excelSSN == "Checked");
    chkbxExcelCC.Checked = (_excelCC == "Checked");
    chkbxExcelWellsFargo.Checked = (_excelWells == "Checked");
}

Answer 4

Use short hand notation for if else

chkbxExcelEnable.Checked = _excelEnable == "Checked" ? true: false;

Answer 5

You could place all your checkboxes on a panel, and set the Tag property of each checkbox to the corresponding key in the registry. Then you could use this code in the form's Load event:

foreach (CheckBox cb in panel1.Controls)
{
    cb.Checked = ((string)Registry.GetValue(RegPath, 
        (string)cb.Tag, "Unchecked") == "Checked");
}

This has the disadvantage of not being immediately obvious to a future maintenance programmer, so I would add a verbose comment here targeted at your eventual replacement.

You could also dispense with the Tag and just name each CheckBox with whatever the corresponding key is, and then use "cb.Name" instead of "(string)cb.Tag". No one likes Hungarian notation anymore, anyway.

Answer 6

private void checkExcelSettings()
    {
        string key = @"HKEY_CURRENT_USER\Software\Mask Data\";

        // Read what the values are for the checkboxes first and assign them to a string.
        string _excelEnable = Convert.ToString(Registry.GetValue(key, "ExcelEnableHash", "Unchecked"));
        string _excelSSN = Convert.ToString(Registry.GetValue(key, "ExcelSSNHash", "Unchecked"));
        string _excelCC = Convert.ToString(Registry.GetValue(key, "ExcelCCHash", "Unchecked"));
        string _excelWells = Convert.ToString(Registry.GetValue(key, "ExcelWellsHash", "Unchecked"));
        string s=@"t\""; //unimportant no-op to placate stackoverflow syntax highlighter.

        // Now let's make sure we reflect what they are supposed to be in the GUI.
        chkbxExcelEnable.Checked = (_excelEnable == "Checked");
        chkbxExcelSSN.Checked = (_excelSSN == "Checked");
        chkbxExcelCC.Checked = (_excelCC == "Checked");
        chkbxExcelWellsFargo.Checked = (_excelWells == "Checked");
    }