patterncsharpMinor
Improving readability of enabling method
Viewed 0 times
readabilityenablingimprovingmethod
Problem
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?
UPDATE
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;
}Solution
A method like this should do at least two things in addition to MrSmith42's answer:
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.... ;-)
Incorporating MrSmith42's suggestion for simplifying the code, it should read:
- It should return boolean to indicate whether it succeeded or not (i.e.
return enable;at the end)
- if it declares a parameter
tryEnable = truethen 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.... ;-)
- You should change the method name to trySetCRUDEnabled(...)
- 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;Code Snippets
bool enable = tryEnable && _securityLevel < MINSECURITY;Context
StackExchange Code Review Q#36514, answer score: 8
Revisions (0)
No revisions yet.