HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Improving readability of enabling method

Submitted by: @import:stackexchange-codereview··
0
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?

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:

  • It should return boolean to indicate whether it succeeded or not (i.e. return enable; at the end)



  • 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.... ;-)

  • 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.