Solved

Help Refactor this code block please

Posted on 2013-06-16
5
252 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 35

Accepted Solution

by:
mccarl earned 250 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 32

Assisted Solution

by:sarabande
sarabande earned 250 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 35

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

Find Ransomware Secrets With All-Source Analysis

Ransomware has become a major concern for organizations; its prevalence has grown due to past successes achieved by threat actors. While each ransomware variant is different, we’ve seen some common tactics and trends used among the authors of the malware.

Join & Write a Comment

What is C++ STL?: STL stands for Standard Template Library and is a part of standard C++ libraries. It contains many useful data structures (containers) and algorithms, which can spare you a lot of the time. Today we will look at the STL Vector. …
Container Orchestration platforms empower organizations to scale their apps at an exceptional rate. This is the reason numerous innovation-driven companies are moving apps to an appropriated datacenter wide platform that empowers them to scale at a …
The viewer will learn how to user default arguments when defining functions. This method of defining functions will be contrasted with the non-default-argument of defining functions.
The viewer will learn additional member functions of the vector class. Specifically, the capacity and swap member functions will be introduced.

759 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

Need Help in Real-Time?

Connect with top rated Experts

18 Experts available now in Live!

Get 1:1 Help Now