Improving readability of enabling method

Question

There doesn't seem to be anything really wrong with this method. But it has a little smell to me. Does the name make sense (do what you think it would do). Is there some logic I could use to make the enabler a bit more elegant?

private void tryEnableCRUD(bool tryEnable = true)
{
 bool enable = false;
 if (_securityLevel < 3)
 {
 enable = tryEnable;
 }
 else
 {
 enable = tryEnable;
 }
 tsbDelete.Enabled = enable;
 tsbReplace.Enabled = enable;
 tsbUpload.Enabled = enable;
}

UPDATE

private void tryEnableCRUD(bool tryEnable = true)
{
 bool enable = false;
 if (_securityLevel < 3)
 {
 enable = tryEnable;
 }
 tsbDelete.Enabled = enable;
 tsbReplace.Enabled = enable;
 tsbUpload.Enabled = enable;
}

• Malachi: please show the entire code in addition to the question. leave what you have but add the rest of the code, you don't want to negate the answers already given
• 200_success: Your Rev 4 seems buggy: your if and else bodies are identical.
• Malachi: why are you setting the parameter? I have been baffled by this the entire time, why bool tryEnable = true' and where is _securityLevel` coming from. looks like you just want to enable everything when you call this function, why are those things not inside the if statement?
• Malachi: when you update code, add it to the end, don't change what you have.
• Mathieu Guindon: Just a note, if this is wpf, this entire method can be dropped. It's winforms, right? (please update tags accordingly)

Answer 1

This method does not make sense, semantically. I am really surprised no one mentioned that. I would refactor it the following way:

private bool TrySetEnabled(bool value)
{
 if (value && _securityLevel < 3) return false;
 tsbDelete.Enabled = tsbReplace.Enabled = tsbUpload.Enabled = value;
 return true;
}

1. I changed the name of method and parameter to emphasize the fact that this method can both disable and enable controls.
2. I changed return type, because it is a common style for methods which start with Try to return the flag which shows if operation succeeded. And it makes sense in this case.

3. I removed this weird code part, where you set enabled to false if security check failed (even when controls were already enabled). This is a really weird behaviour which adds a non-obvious side effect to your method. If you really need this logic just call

    if (!TrySetEnabled(true))
    {
    TrySetEnabled(false);
    } 
   

4. As suggested by MrSmith42 you should probably use constant field instead of 3.

• TruthOf42: I like it. I think some people disagree that methods should have multiple exit points, but to me multiple exits are great.

Answer 2

A method like this should do at least two things in addition to MrSmith42's answer:

1. It should return boolean to indicate whether it succeeded or not (i.e. return enable; at the end)
2. if it declares a parameter tryEnable = true then it should either use it, or remove it.

---

EDIT: after edit and comment from OP.

After your changes I think my point-1 remains valid, but your changes make the 'smell' even worse.... ;-)

1. You should change the method name to trySetCRUDEnabled(...)
2. what if the settings are currently enabled, and the user calls trySetCRUDEnabled(false) ... it makes the logic hard to understand.... in fact, it's broken because now it does not matter what the security-level is ... ;-) (you are changing the code in haste, and making mistakes).

Incorporating MrSmith42's suggestion for simplifying the code, it should read:

bool enable = tryEnable && _securityLevel < MINSECURITY;

• Malachi: i am thinking that this is going to be used in a form or something similar. so it should really only do the last 3 things if it's true. there are probably going to be Drop Down boxes or something that will be enabled if this evaluates to true.
• rolfl: @Malachi - how does that change my suggestions?
• TruthOf42: @rolfl updated question regarding point #2, code now is my original intention
• Malachi: @rolfl, meaning, instead of returning a value to something, it enables the user to be able to do something on the form/page.
• rolfl: @TruthOf42 - updated my answer..... your code is now more broken than before ... ;-)

Answer 3

Based on the code in Rev 5 of the question...

private void tryEnableCRUD(bool tryEnable=true)
{
 tsbDelete.Enabled = tsbReplace.Enabled = tsbUpload.Enabled =
 (_securityLevel < 3) && tryEnable;
}

The first line emphasizes that all three variables will be assigned the same value. The second line emphasizes that that value will be true only when _securityLevel is less than 3 and tryEnable is true. The fact that the resulting function is compact is a bonus.

• Alex Dresko: This is rad. I can think of a million different places I could do something like this.

Answer 4

I would replace the if-else-part:

private void tryEnableCRUD(bool tryEnable = true)
{
 bool enable = (_securityLevel < 3);
 tsbDelete.Enabled = enable;
 tsbReplace.Enabled = enable;
 tsbUpload.Enabled = enable;
}

I would also recommend to make 3 a constant with a speaking name.

private static int SecurityLevelThreshold = 3;
private void tryEnableCRUD(bool tryEnable = true)
{
 bool enable = (_securityLevel < SecurityLevelThreshold);
 tsbDelete.Enabled = enable;
 tsbReplace.Enabled = enable;
 tsbUpload.Enabled = enable;
}

• Alex Dresko: You keep forgetting the = sign on you bool enable line.
• 200_success: "Threshold" is a more appropriate term than "barrier".
• MrSmith42: @200_success: You are right, I changed the name.
• Nikita B: SECURITY_LEVEL_THRESHOLD is C++ naming style. In C# the proper naming is SecurityLevelThreshold.