Want to protect your cyber security and still get fast solutions? Ask a secure question today.Go Premium

x
?
Solved

Help Refactor this code block please

Posted on 2013-06-16
5
Medium Priority
?
283 Views
Last Modified: 2013-06-18
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

0
Comment
Question by:edvinson
  • 2
  • 2
5 Comments
 
LVL 36

Accepted Solution

by:
mccarl earned 1000 total points
ID: 39252299
Firstly, is this an academic assignment? Just so that we know the level to help you...

The first piece of advice that I would give is the consider the final AND operation that you are doing, and realising that it can be thought of in 2 ways. Firstly, the way that most probably think, "if A and B are TRUE the answer is TRUE". But this is equally valid, "if A or B are FALSE the answer is FALSE". Why is this useful?? Well now, if A ends up being FALSE, you don't even need to worry about calculating what B is because you would always end up returning false anyway. Once this sinks in you should realise that you can now replace any of the "pass1 = false" or "pass2 = false" lines with just a "return false".

Once you do this, you should see that there really is no need for pass1 or pass2 at all, and that you can get rid of them. And with the slight re-arrangement of the "if" statements will greatly simplify this method and make it easier to read and understand.

The above is intentionally a little vague, in case this is an academic assignment, I can't give the full code. But either way, it is always good to at first explain the way of thinking and then we can work towards the code the implements that.
0
 
LVL 35

Assisted Solution

by:sarabande
sarabande earned 1000 total points
ID: 39253834
to add to above advices:

if you want to check for a boolean condition - in your case whether the name has an 'a' or not, you better would use bool type rather than size_t for the temporary:

bool has_letter_a = (strComputerName.find('a') != std::string::npos);
....
if (has_letter_a == true)
{
     ...

Open in new window


if you compare that with your code, it is not so much different, but better readable.

also the second boolean whether the checkbox was checked could be made more readable.

....
bool chkbox_a_is_checked = (SendMessage(GetDlgItem(gl_hWnd,IDC_CB_DISABLE_PHONE),BM_GETCHECK,0,0) == BST_CHECKED);

Open in new window


if applying the advice of mccarl you simply could compare the boolean temporaries 'has_letter_a' and 'chkbox_a_is_checked', and if they were different, return with false from function.

Sara
0
 
LVL 1

Author Comment

by:edvinson
ID: 39255280
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.
0
 
LVL 1

Author Closing Comment

by:edvinson
ID: 39255290
@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
0
 
LVL 36

Expert Comment

by:mccarl
ID: 39255439
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

0

Featured Post

Keep up with what's happening at Experts Exchange!

Sign up to receive Decoded, a new monthly digest with product updates, feature release info, continuing education opportunities, and more.

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

When writing generic code, using template meta-programming techniques, it is sometimes useful to know if a type is convertible to another type. A good example of when this might be is if you are writing diagnostic instrumentation for code to generat…
Go is an acronym of golang, is a programming language developed Google in 2007. Go is a new language that is mostly in the C family, with significant input from Pascal/Modula/Oberon family. Hence Go arisen as low-level language with fast compilation…
The goal of the tutorial is to teach the user how to use functions in C++. The video will cover how to define functions, how to call functions and how to create functions prototypes. Microsoft Visual C++ 2010 Express will be used as a text editor an…
The viewer will be introduced to the member functions push_back and pop_back of the vector class. The video will teach the difference between the two as well as how to use each one along with its functionality.

578 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question