What happens when we don't refactor
Tags: codenewbie, dotnet, programming, beginners
Published: 2022-06-27 15:36:43

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