Still celebrating National IT Professionals Day with 3 months of free Premium Membership. Use Code ITDAY17

x
?
Solved

Help Refactor this code block please

Posted on 2013-06-16
5
Medium Priority
?
278 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
[X]
Welcome to Experts Exchange

Add your voice to the tech community where 5M+ people just like you are talking about what matters.

  • Help others & share knowledge
  • Earn cash & points
  • Learn & ask questions
  • 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

On Demand Webinar: Networking for the Cloud Era

Did you know SD-WANs can improve network connectivity? Check out this webinar to learn how an SD-WAN simplified, one-click tool can help you migrate and manage data in the cloud.

Question has a verified solution.

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

Unlike C#, C++ doesn't have native support for sealing classes (so they cannot be sub-classed). At the cost of a virtual base class pointer it is possible to implement a pseudo sealing mechanism The trick is to virtually inherit from a base class…
  Included as part of the C++ Standard Template Library (STL) is a collection of generic containers. Each of these containers serves a different purpose and has different pros and cons. It is often difficult to decide which container to use and …
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 learn additional member functions of the vector class. Specifically, the capacity and swap member functions will be introduced.

705 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