Im working on a dialog box in which several rules must be satisfied before the OK button is enabled.
Currently any action on the page such as entering data or selecting an item from a drop down list (amongst other things) calls a single function called ProcessEvent() - this function handles all logic and either enables or disables the OK button.
My problem is I finding it difficult making the rules concise and understandable.
Some of the rules can be negated by another action on 开发者_如何学Pythonthe dialog and I have now ended up with if else statements all over the place or which are difficult to read and follow & extend.
The code below is a simplification of the problem but demonstrates it well. How do I handle this problem better (If its Possible)
bool CWorkstation::ProcessEvent(void)
{
UpdateData();
CharCount = GetDlgItemInt(IDC_CharCount, NULL, FALSE); //get latest
if ( IsDlgButtonChecked(IDC_USEDBNAME))
{
if (!IsDlgButtonChecked(IDC_MAXDBNAME))
{
EnableNext(TRUE);
}
}
if (IsDlgButtonChecked(IDC_MAXDBNAME) && CharCount)
{
if (IsDlgButtonChecked(IDC_USEXMLNAME))
{
if ( PrefixName.IsEmpty() )
{
EnableNext(FALSE);
}
else
{
EnableNext(TRUE);
}
}
}
if (IsDlgButtonChecked(IDC_USEXMLNAME) && PrefixName.GetLength() > 1)
{
EnableNext(TRUE);
}
if ( IsDlgButtonChecked(IDC_WSAUTONAME) || IsDlgButtonChecked(IDC_RENAMEIFDUP))
{
// TRACE("IDC_WSAUTONAME is Checked\n");
if ( IsDlgButtonChecked(IDC_USEXMLNAME) && PrefixName.GetLength() > 1 )
{
if ( IsDlgButtonChecked(IDC_IDC_USESHORTNAME) )
{
EnableNext(TRUE);
}
else if ( IsDlgButtonChecked(IDC_USELONGNAME) )
{
EnableNext(TRUE);
}
else
{
EnableNext(FALSE);
}
}
if ( !IsDlgButtonChecked(IDC_USEPREFIX) )
{
if ( IsDlgButtonChecked(IDC_IDC_USESHORTNAME) || IsDlgButtonChecked(IDC_USELONGNAME) )
{
EnableNext(TRUE);
}
}
return false;
}
}
I would split your if/else statements into multiple functions, and do an &= on the parameter you send to EnableNext. You should be calling EnableNext only once.
So, for example:
// in CWorkStation::ProcessEvent
bool enableNext = true; // start with true
enableNext &= Condition1(); // of course pick better names than Condition1
enableNext &= Condition2(); // this is just for an example
EnableNext(enableNext);
Where Condition1() might be:
bool Condition1()
{
return (IsDlgButtonChecked(IDC_USEDBNAME)
&& !IsDlgButtonChecked(IDC_MAXDBNAME));
}
And so on.
What's happening here is that the enableNext variable starts with true. Then, each &= you do means that if any of the ConditionX() functions returns false, enableNext will end up false. It will only be true at the end if ALL of the conditions are true.
That problem can be solved with the concept of listeners.
You can make each of your GUI components have a isEnabled()
method, which checks its conditions based on some conditions. The isEnabled()
is called on each GUI component when any action that changes the state of any component is called.
This way you can have the following declarations:
bool CheckBoxComponent::isValid() {
return isNameFilled() && isEmailChecked();
}
bool OkButton::canSend() {
return checkBoxName->isValid() && isEmailChecked();
}
Then, when creating your GUI components you make each of them connect to each other via listener.
This way you have the rules for each component where they belong and you don't have tons of if statements.
It may help to try to formulate the rules as a state-machine, but if that is practical depends on their nature. In that approach, whenever the user fills out some field in the dialog, or checks a checkbox or whatever, you update the state of your sate-machine accordingly. If you have that, you can use Boost.Statechart to implement it.
In cases like that, I tend to make it as simple as possible by (for example) enabling the button by default, and if any other condition is set (or not), disable it; this limit the different cases in "if" conditions with "else".
Restate your condition as a proper boolean statement, properly indent all conditions and add some comments. IMHO, you shouldn't hide the real checks in single-use methods. If you want to comment code, comment it but don't create methods for that purpose, it only obfuscates things and your conditions don't get any simpler:
EnableNext(
// condition 1
IsDlgButtonChecked(IDC_USEDBNAME) && !IsDlgButtonChecked(IDC_MAXDBNAME)
// condition 2
|| IsDlgButtonChecked(IDC_MAXDBNAME) && CharCount
&& IsDlgButtonChecked(IDC_USEXMLNAME) && !PrefixName.IsEmpty()
// condition 3
|| IsDlgButtonChecked(IDC_USEXMLNAME) && PrefixName.GetLength() > 1
// and so on
)
This way it becomes immediately obvious that you seem to check the same condition twice USEXMLNAME && !PrefixName().IsEmpty()
. It is also obvious now, that EnableNext
is always called.
Though it might be a bit "heavier" of a solution than you'd like, you might want to look at Adobe's Adam and Eve libraries. Eve deals with widget layout, and Adam takes a set of statements about the logic of the widgets and puts them together into a controller that enables and disables widgets based on that logic, as well as handling initialization and putting results into the proper variables (e.g., when the user clicks "Ok").
精彩评论