CODEWITHSUNDEEP
c#.netresharper
cardnc
cardnc

Reputation: 93

Refactored with Resharper and bugs were introduced, why?

I had a method which I thought was pretty over the top with if statements, I decided to refactor it with the help of Resharper and later found out it was the cause of a lot of bugs I was having. 

private bool isValid(User user)
{
    if (user == null)
        return false;
    if (user.IsBot)
        return true;
    if (user.GetClient() == null)
        return false;
    if (user.GetClient().GetData() == null)
        return false;
    if (user.GetClient().GetData().CurrentRoomId != _room.RoomId)
        return false;
    return true;
}

I refactored it to this

private bool isValid(User user)
{
    return user?.GetClient() != null && user.GetClient().GetData() != null && user.GetClient().GetData().CurrentRoomId == _room.RoomId;
}

After returning the refactored version to the original all bugs vanished. Just for self improvement purposes, could somebody tell me what I did wrong? I can't see anything but clearly it broke a lot of things so it must have made some difference.

Upvotes: 3

Views: 99

Answers (3)

Matías Fidemraizer
Matías Fidemraizer

Reputation: 64923

There's an alternate and functional approach on which you define rules using lambdas and then you check that all rules are true:

private Func<User, bool>[] Requires = { get; } = new [] 
{ 
      user => user?.isBot == true,
      // Comparing the expression returning Nullable<bool> with true
      // implicitly converts it to bool
      user => (user?.GetClient()?.GetData()?.CurrentRoomId == room.RoomId) == true
}

public bool IsValid(User user) => Requires.All(r => r(user))

That could be also refactored as follows:

public bool isValid(User user) => 
    user?.isBot == true 
    && 
    (user?.GetClient()?.GetData()?.CurrentRoomId == room.RoomId) == true

Upvotes: 0

Praneet Nadkar
Praneet Nadkar

Reputation: 813

If I understand correctly, you have to check for bot if the user object isnt null. And if the user object is null, you have to check for the consequent things like GetClient, GetData etc.

I would write it as :

 static bool IsValid(User user)
 {
    // if the user is not null and IsBot flag is true, return true
    if (user != null && user.IsBot)
       return true;

    // either the user is null or the IsBot flag is false here, hence check the following calls
    return !(user.GetClient() == null || user.GetClient().GetData() == null || user.GetClient().GetData().CurrentRoomId != _room.RoomId);
 }

Please note that you will have to change the code as per your requirement, whether you want true or false to return in which condition.

Upvotes: 0

Daniel Hilgarth
Daniel Hilgarth

Reputation: 174309

The original version was more readable and during your refactor, you introduced a bug. The IsBot check is missing.

You could refactor the method to this:

private bool isValid(User user)
{
    if (user == null)
        return false;
    if (user.IsBot)
        return true;
    return user.GetClient()?.GetData()?.CurrentRoomId == _room.RoomId;
}

Still readable, but shorter and more to the point.

Upvotes: 5

Related Questions