Question 87179 in ASP.NET Question posted in
The official documentation can be found here.

Searching in lists regarding to the database in ASP .NET in C# – Asp.net

this is a description of my task as it should be done

And that is my code. I would be grateful if someone can help me and explain, why my solution is not good.

     public IList<MoneyTransfer> SearchTransfer(string clientAccountNo, decimal? amount, 
        string phrase, string beneficiaryAccountNo)
    {
      var listOfMoneyTransfers = new List<MoneyTransfer>();
      var result = new List<MoneyTransfer>();

      if (clientAccountNo is null)
      {
        return result;
      }
      else
      {
        foreach (BankAccount i in BankAccounts.ToList())
        {
          foreach (MoneyTransfer j in TransfersHistory.ToList())
          {
            if (i.AccountNo == clientAccountNo)
            {
              listOfMoneyTransfers.Add(j);
            }
          }
        }
        if (listOfMoneyTransfers.Count > 0)
        {
          foreach (var k in listOfMoneyTransfers)
          {
            if (k.Amount == amount || k.BeneficiaryAccountNo == beneficiaryAccountNo || k.Title == phrase)
            {
              result.Add(k);
            }
          }
        }
      }

      return result;
    }

Probably that is mistake during searching by three additional parameters, but I am not sure. Please for a help

        private static string CorrectClientAccount = 
          "12345678901234567890000001";

        [Test]
        public void SearchTransferBy_BeneficiaryAccount()
        {
            var service = CreateService();
            service.CreateTransfer(CorrectClientAccount, 250m, 
               "Transfer 1", "12345678901234567890000001");
            service.CreateTransfer(CorrectClientAccount, 300m, "Other 
                2", "12345678901234567890000002");
            service.CreateTransfer(CorrectClientAccount, 400m, "Other 
               3", "12345678901234567890000003");
            service.CreateTransfer(CorrectClientAccount, 500m, "Other 
               4", "12345678901234567890000003");
            var result = service.SearchTransfer(CorrectClientAccount, 
               null, "", "12345678901234567890000003");
            Assert.IsNotNull(result);
            Assert.AreEqual(2, result.Count);
            Assert.IsTrue(result.Any(a => a.ClientAccountNo == 
             CorrectClientAccount && a.Amount == 400m && a.Title == 
             "Other 3" && a.BeneficiaryAccountNo == 
             "12345678901234567890000003"));
            Assert.IsTrue(result.Any(a => a.ClientAccountNo == 
               CorrectClientAccount && a.Amount == 500m && a.Title == 
               "Other 4" && a.BeneficiaryAccountNo == 
               "12345678901234567890000003"));
        }

That is unit test

Tags:

3

Answers


  1. 0

    This part of the code looks problematic:

    foreach (BankAccount i in BankAccounts.ToList())
{
  foreach (MoneyTransfer j in TransfersHistory.ToList())
  {
    if (i.AccountNo == clientAccountNo)
    {
      listOfMoneyTransfers.Add(j);
    }
  }
}

    When looking trough bank accounts and money transfers, you do not check that transfer belongs to a particular account.

    This code would add all of the money transfers to the list, if there is a bank account with this clientAccountNo, and not just the transfers belonging to the client.

    Most likely, you need to do something like this (I can only guess for variable names I’ve not been provided with)

    (this also assumes that MoneyTransfer doesn’t have AccountNo)

    var clientBankAccount = BankAccounts.FirstOrDefault(account => account.AccountNo == clientAccountNo); 
// be sure to check that account has been found

foreach (MoneyTransfer j in TransfersHistory.ToList())
{
  if (j.BankAccountId == clientBankAccount.Id)
  {
    listOfMoneyTransfers.Add(j);
  }
}
    Login or Signup to reply.
  2. 0

    I would first focus on the functional correctness of code and then we can also discuss the the efficiency.

    Functional issue: There is no relationship between BankAccounts and TransferHistory list iterations. We are selecting all the transfer histories without filtering if those originate from the clientBankAccount as we are just matching the clinetAccountNumber with outer iteration item(BankAccount i), all the transfers are included if that condition matches. We should have filtered only those Tranfers that originated from the clientBankAccount.

            foreach (BankAccount i in BankAccounts.ToList())
        {
          foreach (MoneyTransfer j in TransfersHistory.ToList())
          {
            if ***(i.AccountNo == clientAccountNo)***
            {
              listOfMoneyTransfers.Add(j);
            }
          }
        }

    There could have been many optimizations like:
    Adding precondition checks like:

    if (null == amount && string.IsNullOrWhilespace(beneficiaryAccountNo) && string.IsNullOrWhilespace(phrase))
            {
return new List<MoneyTransfer>();
}
    Login or Signup to reply.
  3. 0 
        foreach (MoneyTransfer j in TransfersHistory.ToList())
      {
        if (j.AccountNo == clientAccountNo)
        {
          listOfMoneyTransfers.Add(j);
        }
      }

    You don’t really need the outer loop, and instead be checking for j.ClientAccountNo == clientAccountNo, to get the list of transfers which belong to correct account.

     foreach (var k in listOfMoneyTransfers)
      {
        if (k.Amount == amount || k.BeneficiaryAccountNo == beneficiaryAccountNo || k.Title == phrase)
        {
          result.Add(k);
        }
      }

    This is also an issue, the expression is shortcuited once a condition is met, hence it might result in situations where it returns money transfers which match the amount, but doesn’t match the beneficiary account number or contain the search phrase.

    Once a condition is met, the rest are not evaluated.

    https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/boolean-logical-operators#conditional-logical-or-operator-

    Consider using Linq where clause and conditional searching. something like

    var results = listOfMoneyTransfers
.Where(x => amount.HasValue ? x == amount : true)
.Where(x => string.InNullOrWhiteSpace(phrase) ? x.Title.contains(phrase) : true)
.Where(x => string.InNullOrWhiteSpace(beneficiaryAccountNo) ? x.BeneficiaryAccountNo == beneficiaryAccountNo : true)

    The multiple where clauses will be flatted into a single predicate https://github.com/dotnet/runtime/blob/a24364a09d9aea98b545f16689a53bafc6b18c14/src/libraries/System.Linq/src/System/Linq/Utilities.cs#L56

    Login or Signup to reply.
Please signup or login to give your own answer.

Back To Top
Search