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}) }
            };
        }
    }
}
\$\endgroup\$

closed as off-topic by t3chb0t, yuri, pacmaninbw, esote, IEatBagels Jun 18 at 17:35

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

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – yuri, pacmaninbw, esote
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 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\$

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