C# Update account based on user order data

Question

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);
}

• Emaro: 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.
• TomTom: I added more info
• Emaro: Please add the entire method.
• TomTom: Updated code. And that's it. This is all my code.
• BCdotWEB: Considering that according to your code accountByNumber can be null, bool _isSpecialData = accountByNumber.Vip; would throw an exception in such a case.

Answer 1

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;}
}

• TomTom: I think I will use this option. I'll let you know how I did

Answer 2

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.

Answer 3

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);
 }

• TomTom: I thought of such a variant but I still have too much if-else variation here