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

CodePudding user response：

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

CodePudding user response：

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