What happens when we don't refactor

I've just come across what I think is the most hilariously bad code I've ever seen, and the best part is - it's only 6 lines long with braces, here we go:

private readonly IMemoryCache _cache;

public object Modify(string code, bool Flag)
{
    object result;
    if (_cache.TryGetValue(code.ToLower(), out result))
    {
        return true;
    }

    return Flag;
}

Now the language I work in from day to day is c# so there may be things here that you may not be aware of, but let's break each thing that (I think) is wrong with this snippet and then - let's fix it!

Return Type

Why exactly are we returning object? We know that this is going to return a bool, how do we know that? Well either we return true if this code is in the cache there or we return the Flag whatever that is, but the Flag is a bool.

The Method name

We try and make our methods as brief but as descriptive as possible Modify is brief I'll give it that but if we look inside the method we can see that nothing in there is being modified (now the code that calls this function modifies a property on another class) so we can say that this method name is not descriptive of the logic inside the method.

so now we have to ask ourselves what is this function doing? Well it is checking some sort of cache for a code, if that code exists in the cache then it returns true, if not it returns the value of the flag. so maybe something like CheckCacheForCode I don't know, all I do know is that is a much more descriptive name for this method than Modify so let's stuck to it for now.

The parameters

These are obviously not the names of the original parameters - I had to obfuscate them to talk about this code, because this is commercial software under a non OSS license. But I did try and capture the spirit of the names

Quick suggestion here - pascal case for variable names, so Flag becomes flag, easy

The first line

Now this line might not be as obvious for people who have not touched c# for a while but:

object result;

First of all again with the object - just make it a bool! But why are we defining this here? we could just do:

_cache.TryGetValue(code.ToLower(), out var result)

but then again we're not even using this variable so we can just use a discard:

_cache.TryGetValue(code.ToLower(), out _)

Again this one is a newer feature in c# but it's there so use it.

The if statement

Now we are left with:

if (_cache.TryGetValue(code.ToLower(), out _))
{
    return true;
}

return Flag;

So we can get rid of that if statement:

return _cache.TryGetValue(code.ToLower(), out _) || Flag;

The result

We went from:

private readonly IMemoryCache _cache;

public object Modify(string code, bool Flag)
{
    object result;
    if (_cache.TryGetValue(code.ToLower(), out result))
    {
        return true;
    }

    return Flag;
}

to

public bool CheckCacheForCode(string code, bool flag)
{
    return _cache.TryGetValue(code.ToLower(), out _) || flag;
}

Just by thinking about our code just a little bit.

Thanks all for reading

j