3
\$\begingroup\$

This is a follow-up on bank ATM mockup app in .NET Core framework and Entity Framework Core. I have 2 user interfaces: Console and Web.

Previously I have UOW and repository layer but I have removed them as it is considered overkill by many. Now that I have removed repository layer, just wondering where should I have my FindByCardNoPin method like below? In application layer? Is my exception handling codes considered as good practice? Is this considered as domain-driven design or clean architecture or something else?

Domain Model layer

public class BankAccount
{
    public int Id { get; set; }
    public string AccountName { get; set; }
    public string CardNumber { get; set; }
    public string CardPin { get; set; }
    public decimal Balance { get; set; }

    public decimal CheckBalance()
    {
        return Balance;
    }

    public void Deposit(int amount)
    {
        // Domain logic
        Balance += amount;
    }

    public void Withdraw(int amount)
    {
        // Domain logic
        if (amount > Balance)
        {
            throw new Exception("Withdraw amount exceed account balance.");
        }

        Balance -= amount;
    }
}

    public class Transaction
{
    public int Id { get; set; }
    public int BankAccountId { get; set; }
    public DateTime TransactionDateTime { get; set; }
    public TransactionType TransactionType { get; set; }
    public decimal Amount { get; set; }
}

public enum TransactionType
{
    Deposit, Withdraw
}

Persistence layer

    public class AppDbContext : DbContext
{        
    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        optionsBuilder.UseSqlServer("[connstring]");
    }

    public DbSet<BankAccount> BankAccounts { get; set; }
    public DbSet<Transaction> Transactions { get; set; }
}

public class DatabaseUtility
{
    public void TestDBConn()
    {
        var db = new AppDbContext();

        try
        {
            db.Database.CanConnect();
        }
        catch 
        {
            throw new Exception("System error. Please contact the bank. ");
        }
    }
}


public class RepositoryBankAccount
{
    private readonly AppDbContext context;

    public RepositoryBankAccount()
    {
        context = new AppDbContext();
    }

    public BankAccount FindByCardNoPin(string cardNo, string cardPin)
    {            
        var validUser = (from u in context.BankAccounts
                        where u.CardNumber.Equals(cardNo)
                        && u.CardPin.Equals(cardPin)
                         select u).SingleOrDefault();
        return validUser;
    }

}

Application layer

public class AccountService
{
    public decimal CheckBalanceAmount(int bankAccountId)
    {
        BankAccount bankAccount;

        using (var ctx = new AppDbContext())
        {
            bankAccount = ctx.BankAccounts.Find(bankAccountId);
        }

        if (bankAccount == null)
        {
            throw new Exception("Error");
        }

        return bankAccount.CheckBalance();
    }

    public void DepositAmount(BankAccount bankAccount, int amount)
    {
        using (var ctx = new AppDbContext())
        {
            using (var trans = ctx.Database.BeginTransaction())
            {
                try
                {
                    bankAccount.Deposit(amount);
                    ctx.BankAccounts.Update(bankAccount);

                    var transaction = new Transaction()
                    {
                        BankAccountId = bankAccount.Id,
                        Amount = amount,
                        TransactionDateTime = DateTime.Now,
                        TransactionType = TransactionType.Deposit
                    };

                    ctx.Transactions.Add(transaction);
                    trans.Commit();
                    ctx.SaveChanges();
                }
                catch
                {
                    trans.Rollback();
                }
            }
        }                        
    }

    public void WithdrawAmount(BankAccount bankAccount, int amount)
    {
        using (var ctx = new AppDbContext())
        {
            using (var trans = ctx.Database.BeginTransaction())
            {
                try
                {
                    bankAccount.Withdraw(amount);
                    ctx.BankAccounts.Update(bankAccount);

                    var transaction = new Transaction()
                    {
                        BankAccountId = bankAccount.Id,
                        Amount = amount,
                        TransactionDateTime = DateTime.Now,
                        TransactionType = TransactionType.Withdraw
                    };

                    ctx.Transactions.Add(transaction);
                    trans.Commit();
                    ctx.SaveChanges();
                }
                catch
                {

                    trans.Rollback();
                    throw;
                }
            }
        }                            
    }    
}

User Interface Layer - Console (Note: For brevity, I just show one of the function: Withdraw)

amount = GetAmount();

try
{
    accountService.WithdrawAmount(bankAccount, amount);

    Console.WriteLine($"Withdrew RM {amount} successfully. Please collect your RM {amount} cash. ");
}
catch (Exception ex)
{
    // // todo: write ex.stacktrace into text file for programmer to do troubleshooting.
    Console.WriteLine(ex.Message);
}

User Interface Layer - Web

public IActionResult OnPost()
    {
        if (!ModelState.IsValid)
        {
            return Page();
        }

        var repoBankAccount = new RepositoryBankAccount();

        var validUser = repoBankAccount.FindByCardNoPin(Login.CardNumber, Login.CardPin);

        if (validUser == null)
        {
            ViewData["Error"] = "Invalid card number or card pin.";
            return Page();
        }

        // Create user session.
        HttpContext.Session.SetInt32("userId", validUser.Id);

        return RedirectToPage("/Secure/Dashboard");
    }
\$\endgroup\$
  • 4
    \$\begingroup\$ Is this the previous version of the app? codereview.stackexchange.com/questions/215086/… or this: codereview.stackexchange.com/questions/226423/…? \$\endgroup\$ – dfhwze Aug 25 '19 at 10:23
  • \$\begingroup\$ The second one. That's the latest version before this version. \$\endgroup\$ – Steve Ngai Aug 25 '19 at 13:47
  • 3
    \$\begingroup\$ First Please move the paragraph and the code block at the bottom of the question to the top, it might clarify the question for some people. It might be better to put the FindByCardNoPin where you think it belongs and ask if it is in the correct place, asking where to put it is off-topic. Please provide a link to the previous question. \$\endgroup\$ – pacmaninbw Aug 25 '19 at 14:02
  • 1
    \$\begingroup\$ @pacmaninbw okay. I have moved it up and added my FindByCardNoPin method which I have currently. \$\endgroup\$ – Steve Ngai Aug 26 '19 at 1:09
3
+50
\$\begingroup\$

Domain Model layer

Your domain classes serve 3 conflicting purposes:

  • Data Transfer Object
  • ORM Object (Data Record for EF, some call this a DAO, others wouldn't)
  • Business Entity

For bigger projects, you would like to have 3 different layers exposing equivalent classes. The main problem for your API is that next to having business operations defined that guard the state of the entities, you're also exposing all state through public getters and setters. Let's take a look at BankAccount:

public int Id { get; set; }
public string AccountName { get; set; }
public string CardNumber { get; set; }
public string CardPin { get; set; }
public decimal Balance { get; set; }

Method Withdraw has a business rule defined, albeit hard-coded:

if (amount > Balance)
{
    throw new Exception("Withdraw amount exceed account balance.");
}

But it could easily by cirmcumvented by calling the setter on Balance

someBankAccount.Balance = -1000;

To solve this, you should have a different entity for the persistence layer than in the domain layer and use a mapper between layers.

I would also advise to use use throw hard-coded validation messages, but use some pattern for it (get from DB, from resource files, ..). I would make a custom exception to indicate business errors.

throw new BusinessException(ResourceManager.GetString(Resources.WithDrawExceeded));

Persistence Layer

I am not convinced of the concept to put all entity queryables in a single class AppDbContext. I would let each specific Repository have its own scope of what to manage.

Class DatabaseUtility is a weird one, what would you use it for? Also, since it has no instance state, it should be made static. It's probably a convenience used only internally by the API, so make it internal. Method TestDBConn screems to return a Boolean. If you do decide to throw an error, I would throw some kind of TechnicalException to distinguish technical from business errors. Technical errors could be converted differently to end users, using a generic error message "Something went wrong, please try again later".

RepositoryBankAccount is usually called BankAccountRepository. Method FindByCardNoPin sits at the right location here. An endpoint should call repository.FindByCardNoPin rather than service.FindByCardNoPin. Only when additional business logic is required, the service should provide this method. But even then, the repository should also provide it as input for the service.

Returning SingleOrDefault is ok if your model explicitly guards against multiple matches. But even then, if not found you'll get a generic .NET exception. Invest in your LINQ-style methods that take a friendly message on not found, too many found, etc.

Application Layer

This layer is pretty well implemented. It handles transactions, calls to repositories and business checks.

Method CheckBalanceAmount throws a really lame exception. Clean this up a bit.

if (bankAccount == null)
{
    throw new Exception("Error");
}

Method DepositAmount has different behavior than WithdrawAmount concerning transaction rollback. The former performs trans.Rollback(); while the latter also rethrows the exception throw;. The latter is better practice. You should try to invest in a solution that handles transactions as an aspect instead, this to avoid boiler-plate code.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for all the code review and comments :) For the exception, I guess I have to create another class library with both TechnicalException and BusinessException right? By the way, there is a issue I encounter. When I try to enter withdraw amount more than the account balance, it did throw the exception. But the second time, I enter the withdraw amount, the session became null. But if I throw my BusinessException in the Application layer, it has no issue. \$\endgroup\$ – Steve Ngai Aug 28 '19 at 0:12
  • \$\begingroup\$ and for 'For bigger projects, you would like to have 3 different layers exposing equivalent classes', do you have any links with example? \$\endgroup\$ – Steve Ngai Aug 28 '19 at 1:09
  • 1
    \$\begingroup\$ Here's a small summary: bloomlab.blogspot.com/2014/05/…. You can also lookup each type of object individually at wikipedia to see what they are meant for. \$\endgroup\$ – dfhwze Aug 28 '19 at 4:25
  • 1
    \$\begingroup\$ regarding: I would let each specific Repository have its own scope of what to manage. - this only works with CRUD (which is in real-world rarely the case) as soon as you need a cross-repository query you're screwed :-P I always build a single repo for everything. The crud-repository pattern is a like virus distroying applications. \$\endgroup\$ – t3chb0t Aug 28 '19 at 16:42
  • \$\begingroup\$ What do you think of creating two more derived Transaction types for Deposit and Withdraw where each of them would check the amount aginst being postiv or negative? Currently OP is using an enum for that. \$\endgroup\$ – t3chb0t Aug 28 '19 at 16:48

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.