2
\$\begingroup\$

The method works, but I would like to know if there is any way to make it more readable, optimized?

I have user data (i want to import/update it). Accounts is finded by user data.

========

User order data is entered into the system.

userData.Id - user Id; userData.OrderNumber - user order number.

The "UpdateAccount" method below is used by the user. The user enters their Id and OrderNumber into the system (there are FirstName, LastName, etc., but they are irrelevant here because they are used in the UpdateAllUserDataToAccount method). Therefore, it is necessary to discover whether an account with the relevant data exists in the system.

You can update account data if the account has userData.OrderNumber or userData.Id. If no account under userData.OrderNumber or userData.Id is found in the system then create a new account and update its data. Do not allow anything if userData.OrderNumber, userData.Id is in different account (can not exist in the system in different account).

An account can have userData.OrderNumber in the system and not userData.Id because the employee registered the parcel but did not have a user id. An account can have userData.Id in the system and not userData.OrderNumber because the user has previously been registered in the system.

The user data userData.OrderNumber and userData.Id are unique and belong to only one user.

bool _isSpecialData - to indicate that the user is special (set before this code).

private void UpdateAccount(UserModel userData)
{               
    Account accountById = _accountController.GetAccountById(userData.Id);
    Account accountByNumber = _accountController.GetAccountByNumber(userData.Number);
    bool _isSpecialData = accountByNumber != null ? accountByNumber.Vip : false;
    
    if (accountById != null)
    {
        if (accountByNumber != null)
        {
            if (accountById.Id == accountByNumber.Id)
            {
                if (_isSpecialData)
                {
                    AddPartUserDataToAccount(userData, accountByNumber);

                    if (userData.Status == Blocked) return;
                }
            }
            else
            {
                _log.Error($"User data can not be in different accounts");
                return;
            }
        }
        else
        {
            AddPartUserDataToAccount(userData, accountById);
        }
    }
    else
    {
        if (accountByNumber != null)
        {
            if (accountByNumber.Id == null)
            {                            
                accountByNumber.Id = userData.Id;

                if (_isSpecialData)
                {
                    AddPartUserDataToAccount(userData, accountByNumber);
                    if (userData.Status == Blocked) return;
                }
            }
            else
            {
                _log.Error($"accountByNumber.Id can be just with the same value as userData.Id or with null (because it was not set in first place)");
                return;
            }
        }
        else
        {
            CreateNewAccount(userData);
        }
    }
                
    UpdateAllUserDataToAccount(userData);
}
\$\endgroup\$
7
  • 1
    \$\begingroup\$ Can you please provide more of the code? It's not clear, what types variables like accountById, accountByNumber etc. are. This could be useful. At least post the entire method. \$\endgroup\$ – Emaro Sep 22 '20 at 12:41
  • \$\begingroup\$ I added more info \$\endgroup\$ – TomTom Sep 22 '20 at 13:02
  • \$\begingroup\$ Please add the entire method. \$\endgroup\$ – Emaro Sep 22 '20 at 13:31
  • \$\begingroup\$ Updated code. And that's it. This is all my code. \$\endgroup\$ – TomTom Sep 22 '20 at 14:09
  • \$\begingroup\$ Considering that according to your code accountByNumber can be null, bool _isSpecialData = accountByNumber.Vip; would throw an exception in such a case. \$\endgroup\$ – BCdotWEB Sep 22 '20 at 15:54
2
\$\begingroup\$

well What I would do is created model from the if of validations

you have 3 path in the if

when accountById is null and accountByNumber is null
when accountById is not null
and the else

Look at the code and see how the TypeCase property let identify each case

using your if statement

private void UpdateAccount(UserModel userData)
{               
  var result = ProcessUpdateAccount(userData);

  if (result.HasError) _log.Error(result.ErrorText);

  if (result.AllowGlobalUpdate) UpdateAllUserDataToAccount(userData);
}

the main function

Public UpdateResult ProcessUpdateAccount(UserModel userData){

  var firsAccount  = _accountController.GetAccountById(userId);
  var secondAccount = _accountController.GetAccountByNumber(userNumber);

  if (firstAccount == null && secondAccount == 0)
  {
    CreateNewAccount(userData);

    return new UpdateResult() { TypeCase = 1, Title = "new account", HasError = false, ErrorText = "", AllowGlobalUpdate = true };
  }

  if (firstAccount != null) return FirstAccountPath(userData, firstAccount, secondAccount);

  //second account path
  return SecondAccountPath(userData, secondAccount);    

}

so accountById Path

Public UpdateResult FirstAccountPath(
  UserModel userData,
  Account firstAccount, 
  Account secondAccount){

  if (secoundAccount == null)
  {
     AddPartUserDataToAccount(userData, firstAccount,);
     return new UpdateResult() { TypeCase = 2, Title = "First OK Second Not Exists", HasError = False, ErrorText = "", AllowGlobalUpdate = true };
  }

  if (firstAccount.Id != secoundAccount.Id)
  {
    return new UpdateResult() { TypeCase = 3, Title = "", HasError = true, ErrorText = "User data can not be in different accounts", AllowGlobalUpdate = false };
  }

  if (seconAccount.Vip){
    AddPartUserDataToAccount(userData, secondAccount);

    if (userData.Status == Blocked) 
        return new UpdateResult() { TypeCase = 4, Title = "", HasError = false, ErrorText = "", AllowGlobalUpdate = false };     
    
    return new UpdateResult() { TypeCase = 5, Title = "VIP", HasError = false, ErrorText = "", AllowGlobalUpdate = true };     

  }

  //No case in code path
  return new UpdateResult() { TypeCase = 6, Title = "No Case in code Path", HasError = false, ErrorText = "", AllowGlobalUpdate = true };     

}

and finally

Public UpdateResult SecondAccountPath(
  UserModel userData,
  Account secondAccount){

  if (secondAccount.Id != null)
  {
     return new UpdateResult()  { TypeCase = 7, Title = "", HasError = true, 
     ErrorText = "accountByNumber.Id can be just with the same value as userData.Id or with null (because it was not set in first place)", 
     AllowGlobalUpdate = false };
  }

 secondAccount.Id = userData.Id;

 if (seconAccount.Vip)
 {
    AddPartUserDataToAccount(userData, secondAccount);
    if (userData.Status == Blocked) 
         return new  UpdateResult(){ TypeCase = 8, Title = "", HasError = false, ErrorText = "", AllowGlobalUpdate = false };     

    return new UpdateResult() { TypeCase = 9, Title = "", HasError = false, ErrorText = "", AllowGlobalUpdate = true };     
 }

   //No case in code path
  return new  UpdateResult() { TypeCase = 10, Title = "No Case in code Path", HasError = false, ErrorText = "", AllowGlobalUpdate = true };     

}

so the UpdateResult class

Public Class UpdateResult
{
  public int TypeCase {get;set;}
  public Title TypeCase {get;set;}
  public HasError TypeCase {get;set;}
  public ErrorText TypeCase {get;set;}
  public AllowGlobalUpdate TypeCase {get;set;}
}
\$\endgroup\$
1
  • \$\begingroup\$ I think I will use this option. I'll let you know how I did \$\endgroup\$ – TomTom Sep 28 '20 at 6:13
2
\$\begingroup\$

This code is hard to read, deeply nested and hard to understand. But you already know that.

Since you have a lot of binary cases (if-else) I suggest creating a truth table on paper or whiteboard https://en.wikipedia.org/wiki/Truth_table#Binary_operations for your different combinations of input and output.

This should help you figure out which cases can be simplified, short-circuit, etc, and then you can rewrite the code to make it more readable. Use early returns and maybe refactor it into functions that allow you to check error conditions and return/exit as soon as possible for each case.

The end goal should be a code structure that doesn't nest 4 5 levels deep, preferrably no more than 2.

\$\endgroup\$
2
\$\begingroup\$

I think it can be something like this, also it can be improved by using variable names that reflects your business conditions better.

private void UpdateAccount(UserModel userData)
    {
        Account accountById = _accountController.GetAccountById(userData.Id);
        Account accountByNumber = _accountController.GetAccountByNumber(userData.Number);
        bool _isSpecialData = accountByNumber != null ? accountByNumber.Vip : false;
        bool isBothAccountByIdAndNumberExistsAndMatch = accountById != null && accountByNumber != null && accountById.Id == accountByNumber.Id;
        bool isBothAccountByIdAndNumberExistsAndNotMatch_Exception = accountById != null && accountByNumber != null && accountById.Id != accountByNumber.Id;
        bool isAccountByIdOnlyExists = accountById != null && accountByNumber == null;
        bool isAccountByNumberOnlyExistsWithEmptyId =  accountByNumber != null && accountByNumber.Id == null && accountById == null;
        bool isAccountByNumberOnlyExistsWithNonEmptyId_Exception = accountByNumber != null && accountByNumber.Id != null && accountById == null;
        bool shouldCreateNewAccount = accountById == null && accountByNumber == null;


        if (isBothAccountByIdAndNumberExistsAndNotMatch_Exception)
        {
            _log.Error($"User data can not be in different accounts");
            return;
        }
        else if (isAccountByNumberOnlyExistsWithNonEmptyId_Exception)
        {
            _log.Error($"accountByNumber.Id can be just with the same value as userData.Id or with null (because it was not set in first place)");
            return;
        }
        else if (shouldCreateNewAccount)
        {
            CreateNewAccount(userData);
        }
        else if (isAccountByIdOnlyExists)
        {
            AddPartUserDataToAccount(userData, accountById);
        }
        else if (isAccountByNumberOnlyExistsWithEmptyId)
        {
            accountByNumber.Id = userData.Id;
            if (_isSpecialData)
            {
                AddPartUserDataToAccount(userData, accountByNumber);
                if (userData.Status == "Blocked") return;
            }
        }
        else if (isBothAccountByIdAndNumberExistsAndMatch && _isSpecialData)
        {
            AddPartUserDataToAccount(userData, accountByNumber);
            if (userData.Status == "Blocked") return;
        }
        UpdateAllUserDataToAccount(userData);
    }
\$\endgroup\$
1
  • \$\begingroup\$ I thought of such a variant but I still have too much if-else variation here \$\endgroup\$ – TomTom Oct 6 '20 at 9:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.