1
\$\begingroup\$

I am creating a Expense Tracking Application in C# and right now I am trying to build its building blocks in a console app. I Would like to know your feedback if it is in a Object Oriented Design/Structure.

namespace ExpenseTrackerOOP
{
    class ExpenseTracker
    {
        static void Main(string[] args)
        {
            Account expense = new Expenses();
            expense.Initialize();

            Account earnings = new Earnings();
            earnings.Initialize();

        }
    }


    public enum ExpenseType
    {
        Food,
        Transportation,
        Utitlities
    }
    public enum AccountTypes
    {
        Expenses,
        Earnings
    }
    public class AccountType
    {
        public AccountTypes Type;
    }

    public abstract class Account
    {
        public decimal Value { get; set; }

        public AccountType Type;

        public abstract void Initialize();
    }

    public class Expenses : Account
    {
        public List<Expenses> Expenses_list { get; set; }

        public override void Initialize()
        {
            Expenses_list = new List<Expenses>(){
                new Expenses() { Value = 10, Type = (new AccountType{Type = AccountTypes.Expenses}) },
                new Expenses() { Value = 20, Type = (new AccountType{Type = AccountTypes.Expenses}) },
                new Expenses() { Value = 30, Type = (new AccountType{Type = AccountTypes.Expenses}) }
            };
        }
    }


    public class Earnings : Account
    {
        public List<Earnings> Earnings_list { get; set; }

        public override void Initialize()
        {
            Earnings_list = new List<Earnings>(){
                new Earnings() { Value = 10, Type = (new AccountType{Type = AccountTypes.Earnings}) },
                new Earnings() { Value = 20, Type = (new AccountType{Type = AccountTypes.Earnings}) },
                new Earnings() { Value = 30, Type = (new AccountType{Type = AccountTypes.Earnings}) }
            };
        }
    }
}
New contributor
doctorWeird is a new contributor to this site. Take care in asking for clarification, commenting, and answering. Check out our Code of Conduct.
\$\endgroup\$
  • 2
    \$\begingroup\$ Please tell us more about how it works, what each block is supposed to do etc. \$\endgroup\$ – t3chb0t Jun 12 at 13:42
  • 2
    \$\begingroup\$ ExpenseType isn't being used in this code. Why does an Expenses account have a list of Expenses accounts? Are you confusing your expense accounts class with actual expenses? You'll need two separate classes to represent those (at least normally). \$\endgroup\$ – Shelby115 Jun 12 at 18:30
5
\$\begingroup\$

There are several aspects about your code worth reviewing: OO-principles, code conventions, functional design. Since you specifically requested to review OO-design and your question lacks context to review functional design, I'll focus on the former.


Inheritance

Expenses and Earnings both derive from Account to provide their specific implementation of Initialze. Because of a lack of context, I can't make a call about this design. The implementation of both classes does seem off. I don't think you want each instance of these classes to create and store the same fixed list of presets.


Polymorphism

This is proper use of polymorphism. The declaring type Account is the closest fit to enable the use case Initialize. The instance type Expenses provides the specific implementation of that method.

Account expense = new Expenses();
expense.Initialize();

Composition

You have been a bit too diligent in the composition of some of your classes. AccountType is a redundant class, which acts only as a wrapper for AccountTypes Type. Get rid of it and use AccountTypes directly in Account.

public class AccountType
{
     public AccountTypes Type;
}

Encapsulation

Account is an abstract class that does not provide an explicit constructor. A public default constructor is implied. It is impossible for derived classes to reduce the scope of the access modifier, hence it's a breach in encapsulation. Consider providing a protected constructor for abstract classes.

public abstract class Account
{
    protected Account() {}
}

All your properties and fields are mutable. This might be as desiged, but at least think about whether this is a requirement or just lazy design.

public decimal Value { get; set; }

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks a lot for your code review. Appreciate it. \$\endgroup\$ – doctorWeird Jun 13 at 6:51
3
\$\begingroup\$

You should consider the possibility to make paradoxical objects. The below is actually possible and valid according to your classes and their properties but makes no sense:

Account account = new Expenses { Value = 100.00m, Type = new AccountType { Type = AccountTypes.Earnings } }

In other words: When you have a class hierarchy that defines the data types, there is no need to and it can lead to paradoxical objects having a property that defines the same as the containing class. So an Expenses account is an expenses account by it own definition as subclass to Account and doesn't need the Type property. So get rid of that completely. If you stand on it, you'll have to make it read only and hard code the Type Property on each of the subclasses:

public abstract class Account
{
  public abstract AccountTypes Type { get; }
}

public class Expenses
{
  public override AccountTypes Type => AccountTypes.Expenses;
  etc...

Alternatively you can remove the subclasses to Account and have an AccountTypes member as defining property, but then it isn't much of a class hierarchy.


\$\endgroup\$

Your Answer

doctorWeird is a new contributor. Be nice, and check out our Code of Conduct.

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.