Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am attempting to design an application that tracks currency and recent transactions in a safe. I am using entity-framework code-first in a wpf desktop application.

The first goal is to be able to track whats currently in the safe. For example - 87 Ones, 55 Twos, 76 fives, etc..

The second goal is to be able to track recent deposits/withdrawals into/out of the safe. For example - user X deposited 5 ones, 10 fives on 1/1/14.

Is the following a reasonable entity framework data model to accomplish this?

class Safe
{
    public int SafeId { get; set; }

    public CurrencySet Totals { get; set; }

    public ICollection<SafeTransaction> SafeTransactions { get; set; }
}

class CurrencySet
{
    public int CurrencySetId { get; set; }

    public int Ones { get; set; }
    public int Twos { get; set; }
    public int Fives { get; set; }
    public int Tens { get; set; }
    public int Twenties { get; set; }
    public int Fifties { get; set; }
    public int Hundreds { get; set; }
}

class SafeTransaction
{
    public int SafeTransActionId { get; set; }

    public TransactionType TransactionType { get; set; }

    public User User { get; set; }

    public CurrencySet Currency { get; set; }

    public DateTime TimeStamp { get; set; }

}

enum TransactionType
{
    Withdrawal,
    Deposit
}

One concern I have is tracking non-US currency. Not a requirement currently, but maybe in the future.

share|improve this question

closed as off-topic by Mat's Mug, Edward, 200_success Jul 19 at 22:46

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. Such questions may be suitable for Stack Overflow or Programmers. After the question has been edited to contain working code, we will consider reopening it." – 200_success
If this question can be reworded to fit the rules in the help center, please edit the question.

    
I don't think that this code is sufficiently complete to be reviewable. –  200_success Jul 19 at 19:23
    
This question appears to be off-topic because improving data model design changes what the code does; code review answers normally improve how code does what it does, without changing what it does. –  Mat's Mug Jul 19 at 20:06
    
Where would the more appropriate place for a question like this be? –  dan Jul 19 at 20:45

1 Answer 1

up vote 4 down vote accepted

One concern I have is tracking non-US currency. Not a requirement currently, but maybe in the future.

You're right to be concerned about the flexibility of your approach: what would Twos return in a currency that doesn't have toonies?

Taking future considerations into account, I think you need to re-think your model.


CurrencySet

By modeling the CurrencySet entity the way you have, you've pretty much hard-coded the currency paper's nominal values:

class CurrencySet
{
    public int CurrencySetId { get; set; }

    public int Ones { get; set; }
    public int Twos { get; set; }
    public int Fives { get; set; }
    public int Tens { get; set; }
    public int Twenties { get; set; }
    public int Fifties { get; set; }
    public int Hundreds { get; set; }
}

I'd suggest something like this:

public class Currency
{
    public int Id { get; set; }
    public string Code { get; set; }
    public string CultureName { get; set; } // for number formats

    public virtual ICollection<CurrencyExchangeRate> CurrencyExchangeRates { get; set; }
    public virtual ICollection<CurrencyPaper> CurrencyPapers { get; set; }
}

public class CurrencyExchangeRate
{
    public int Id { get; set; }
    public int CurrencyId { get; set; }
    public int ReferenceCurrencyId { get; set; }

    public decimal Rate { get; set; }

    public virtual Currency Currency { get; set; }
    public virtual Currency ReferenceCurrency { get; set; }
}

public class CurrencyPaper
{
    public int Id { get; set; }
    public int CurrencyId { get; set; }

    public decimal NominalValue { get; set; }
    public string SerialNumber { get; set; }
    public string Issuer { get; set; }
    public int YearIssued { get; set; }

    public virtual Currency Currency { get; set; }
}

Now you can detect currency paper with consecutive serial numbers if you want, and your Safe can contain papers in many currencies, and calculate the total value in $USD, CDN$, €EUR or ¥YEN, given the exchange rate for $USD as a reference currency.

Granted, me putting SerialNumber, Issuer and YearIssued in there is pushing it, but it makes a point: this model is much closer to the actual real-world objects - an approach with its own pros and cons.


Safe

Doing this turns your Safe into an actual safe that literally contains money - it also means the calculating the Totals is a method that has nothing to do in the data itself:

public class Safe
{
    int Id { get; set }

    //...

    public virtual ICollection<CurrencyPaper> CurrencyPapers { get; set; }
    public virtual ICollection<TransactionLog> TransactionLogs { get; set; }
}

SafeTransaction

It's really a log entry more than the transaction itself: I think I'd call it TransactionLog though.

Now what your code is doing, is logging the number of individual CurrencyPaper instances that were involved in a transaction. To do this with the approach I'm suggesting, you might want to break the transaction logs in two tables for full details: one with "header", another with "details" (enabling tracking of individual CurrencyPaper instances) - but that's certainly overkill. You could also just track the total amount deposited/withdrawn, given a Currency.


  • Navigation properties should be virtual
  • Be consistent about FK properties - your SafeTransaction doesn't have them.
  • Naming is ok - although I prefer the entity key to be called Id.
share|improve this answer

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