asked on

Help Refactor this code block please

I think my code shows alot of redundancy. I could use some help.

I have two checkboxes.

1. If COMPUTERNAME contains an 'a', then CheckBox 1 should be checked, otherwise should NOT be checked.

2. If COMPUTERNAME contains an 'o', Checkbox2 should be checked, otherwise not checked.

If both conditions are correct, return TRUE. Otherwise, FALSE.

That's it in a nutshell. Here is my MESSY code. Can you help me refactor this ?

bool Task2()
{
       bool pass1, pass2, pass3 = false;
       
       string strComputerName(lpszComputer);
       
       size_t found = strComputerName.find('a');
       size_t found2 = strComputerName.find('o');
       LRESULT cb1 = SendMessage(GetDlgItem(gl_hWnd,IDC_CB_DISABLE_PHONE),BM_GETCHECK,0,0);
       LRESULT cb2 = SendMessage(GetDlgItem(gl_hWnd,IDC_CB_WEAR_MASK),BM_GETCHECK,0,0);
       
       //IDC_CB_DISABLE_PHONE
       if(found!=std::string::npos){
            //a found in computer name
            if(cb1 == BST_CHECKED)
            {
                pass1 = true;
               } else {
               pass1 = false;
            }                        
       } else {
            //No a in Computer Name
            if(cb1 == BST_CHECKED)
            {
                pass1 = false;                                                                      
            } else {
               pass1 = true;    
            }
                   
       }
       
       // Now the second checkbox
       if(found2!=std::string::npos){
            //o found in computer name
            if(cb2 == BST_CHECKED)
            {
               pass2 = true;
               } else {
               pass2 = false;
            }                        
       } else {
            //No o in Computer Name
            if(cb2 == BST_CHECKED)
            {
               pass2 = false;                                                                      
            } else {
               pass2 = true;    
            }
                   
       }
  
       // Did they pass both tests????
       if(pass1 && pass2){
                   return true;
       }
                
       return false;
}

Open in new window

ASKER CERTIFIED SOLUTION

mccarl

membership

This solution is only available to members.

To access this solution, you must be a member of Experts Exchange.

Start Free Trial

SOLUTION

sarabande

membership

This solution is only available to members.

To access this solution, you must be a member of Experts Exchange.

Start Free Trial

edvinson

ASKER

This is not an academic assignment, lol, at 41 those days are long behind me. :)

Thanks for the advice. I will combine both suggestions to optimize my code.

edvinson

ASKER

@mccarl, I would love to see your version of this code. Although I think I understand your idea, it would help to see it. Thanks

mccarl

Well with the help of Sara's advice too, about making those boolean variables, the code has been pretty much already described in words. So, if you have the four booleans (has_letter_a, has_letter_o, chkbox_1_checked, chkbox_2_checked) correctly assigned, then you just need this small piece of code...

if (has_letter_a != chkbox_1_checked) {
    return false;
}

if (has_letter_o != chkbox_2_checked) {
    return false;
}

return true;

Open in new window

Now since those if conditions have been simplified quite a bit, the following "identical in function but even simpler" code, may be appropriate (leave it up to you as to what is more "readable" as that is a bit of personal preference)...

return ((has_letter_a == chkbox_1_checked) && (has_letter_o == chkbox_2_checked));

Open in new window