DEV Community

Mark Davies
Mark Davies

Posted on

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;
}
Enter fullscreen mode Exit fullscreen mode

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;
Enter fullscreen mode Exit fullscreen mode

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)
Enter fullscreen mode Exit fullscreen mode

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

_cache.TryGetValue(code.ToLower(), out _)
Enter fullscreen mode Exit fullscreen mode

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;
Enter fullscreen mode Exit fullscreen mode

So we can get rid of that if statement:

return _cache.TryGetValue(code.ToLower(), out _) || Flag;
Enter fullscreen mode Exit fullscreen mode

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;
}
Enter fullscreen mode Exit fullscreen mode

to

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

Just by thinking about our code just a little bit.

Thanks all for reading

j

Oldest comments (0)