Sales tax calculator, rejected for being not OOP

Question

I applied for an Application Developer position. They require all their applicants to complete 1 of 3 programming assignments. I picked one for sales tax calculation. It was quite simple.

Write a program the feeds in prices. If the item is a Book, Food or Medical product there is no tax, if not there is a 10% tax (other item). If the item is an import there is a 5% tax (it's possible to have an import and "other item" type of item with 15% tax (10 + 5)). The program needs to print receipts like this...

• Book: 9.12 (2 @ 4.56)
• Imported dog food: 6.52
• Perfume: 6.23
• Sales Taxes: 1.65
• Total: 21.87

You can see in this case the application can "group" items and provide tax calculation including total tax.

To describe it my application had a menu the continuing prompting the user with information until they broke from the menu. The product information was placed into a List. A lambda expression is written against the list to group and get the duplicate items. An if statement nested within a foreach statement fires the tax calculation to print out the receipts.

namespace Cash
{    

    public class product
    {
        public string prod_name { get; set; }
        public decimal prod_price { get; set; }
        public bool is_import { get; set; }
        public bool is_other { get; set; }
    }


    class Program
    {

        static void Main(string[] args)
        {
            Program prog = new Program();
            prog.menu();            
        }

        public void menu()
        {
            string men1;
            string prod_name=null;
            bool is_import;
            bool is_other;
            decimal prod_price;

            var prod_list = new List<product>();

            while (true)
            {
                Console.WriteLine("Sales Tax");
                Console.WriteLine();
                Console.WriteLine("***Please Select the Item Type***");
                Console.WriteLine("1. Book, Food or Medical Product");
                Console.WriteLine("2. Other");
                Console.WriteLine("Please select a menu option or 0 to end:");
                men1 = Console.ReadLine();


                if (men1.Equals("0"))
                {
                    Print(prod_list);
                    break;
                }

                Console.WriteLine("Is this product an import? (Y/N)");

                is_import = (Console.ReadLine().ToUpper().Equals("Y")) ? true : false;

                is_other = (men1.Equals("2")) ? true : false;

                Console.WriteLine("***Please enter the price***");
                prod_price = Convert.ToDecimal(Console.ReadLine());

                Console.WriteLine("Please enter the descriptive name of the product");
                prod_name = Console.ReadLine();

                prod_list.Add(new product { is_import = is_import, prod_name = prod_name, prod_price = prod_price, is_other = is_other });               
            }
        }

        public void Print(List<product> prod_list)
        {

            var result = prod_list.GroupBy(x => new { x.prod_name, x.prod_price, x.is_other, x.is_import })
                                             .Select(x => new
                                             {
                                                 prod_name = x.Key.prod_name,
                                                 sum = x.Sum(z => z.prod_price),
                                                 count = x.Count(),
                                                 prod_price = x.Key.prod_price,
                                                 is_other = x.Key.is_other,
                                                 is_import = x.Key.is_import,                                                 
                                             });

            foreach (var item in result)
            {
                if (item.count > 1)
                {
                    Console.WriteLine(item.prod_name + ": " + string.Format("{0:0.00}", CalcTax(item.sum, item.is_other, item.is_import)) + " (" + item.count + " @ " + item.prod_price + ")");
                }

                else
                {
                    Console.WriteLine(item.prod_name + ": " + string.Format("{0:0.00}", CalcTax(item.sum, item.is_other, item.is_import)));
                }

            }


            Console.WriteLine("Sales Taxes: " + string.Format("{0:0.00}", Total_tax));
            Console.WriteLine("Total: " + string.Format("{0:0.00}", prod_list.Sum(item => item.prod_price) + Total_tax));
            Console.ReadLine();            
        }

        public decimal CalcTax(decimal price, bool is_other, bool is_import)
        {
            decimal _price=0;

            if (is_import == true && is_other == true)
            {
                _price = customRound((price * 15) / 100) + price;
                Total_tax = customRound((_price - price));
            }

            if(is_import == true && is_other == false)//
            {
                _price = customRound((price * 5) / 100) + price;
                Total_tax = customRound((_price - price));
            }

            if (is_import == false && is_other == true)
            {
                _price = customRound((price * 10) / 100) + price;
                Total_tax = customRound((_price - price));
            }

            if (is_import == false && is_other == false)
            {
                _price = price;
                Total_tax = customRound((_price - price));
            }

            return _price;
        }

        private decimal _Total_tax;
        public decimal Total_tax
        {
            get
            {
                return this._Total_tax;
            }

            set
            {
                this._Total_tax = value + Total_tax;
            }
        }   

        public decimal customRound(decimal num)
        {
            return Math.Round(num * 20.0M, MidpointRounding.AwayFromZero) / 20.0M;
        }
    }
}

After submitting this to the company the recruiter came back and informed me that the company indicated that this "was not object oriented". So either I'm pretty way off or I could make some adjustments to meet their requirements. Anyway, I performed the work in 2 days (time limit).

How could this be written to follow OOP concepts? I'm not asking someone to "re-write" this for me. I think I have the right frame of mind. I just need someone to "nudge" me in the right direction.

Answer 1

I guess what they were looking for was methods on classes. Also I recommend you download resharper and see what it says about your naming conventions as they are non standard.

A 'Classic OO' as I call it approach might look a bit like

 public class Product
    {
        public string Name { get; private set; }
        public decimal BasePrice { get; private set; }
        public bool IsImport { get; private set; }

        /// <summary>
        /// enum of book, food etc
        /// </summary>
        public ItemType Type { get; private set; }

        /// <summary>
        /// constructor allows you to set properties
        public Product(string name, decimal basePrice, bool isImport, ItemType Type)
        {
        }

        /// <summary>
        /// business logic to work out sale price from private fields
        /// </summary>
        /// <returns></returns>
        public decimal GetSalePrice()
        {
            //do calculation
        }
    }

Also, as another commenter notes. putting some inheritance in wouldn't hurt even though its not really required. The point of these interview tests is to demonstrate you know about stuff. Its assumed you will be able to solve the problem.

As a guide, I always include the following :

• Unit Tests - cannot stress this enough! Names match tests cases ie. givenXThenY (TDD)
• Classes with names matching business terms (DDD)
• Methods on those classes (OO)
• Service classes with business logic (SoA)
• Interfaces for EVERYTHING (more OO/DI) also shows inheritance knowledge*
• injection of services into constructors via interfaces (DI)
• ViewModel objects or some other separation of display (MVVM)
• Repository object (repository patter/separation of datalayer)
• At least one use of Generic Collections
• At least one use of Linq
• XML Comment everything (code readablity)
• Method Names which describe what they do (Clean Coding)
• More than one project per solution (not including test project)
• Name projects like name spaces ie YourCompany.Example5.Models

other stuff you can throw in if you think the question is angling for it

• Recursion
• abstract classes/ more inheritance
• the modulus operator % some tests angle for this, shows you know maths?
• async method and Task, multithreading

*When I say EVERYTHING I mean Services AND Models, really you don't need both, but some people like one and some the other. A lot of these things are contradictory or double up on patterns. The point is not writing good code, its demonstrating you know about lots of patterns and coding practices etc

Answer 2

I think they were probably looking for you to split out the responsibilities into classes.

You created your product class to encapsulate the data but that's where you stopped. You stuffed everything to do with UI (writing/reading the console) and calculating the tax in the main Program class.

You at least need a separate class for calculating tax:

public class TaxCalculator
{
    public decimal CalculateTax(Product product)
    {
         // Some code - everyone loves political humour...
         return isMultinationalOrganisation ? 0 : 10000m;
    }
} 

Other than that there's some style points that could rely do with some work:

Classes should be named in Pascal case (upper camel case). product should be Product.

Methods should also be Pascal case customRound should be CustomRound.

What the hell is customRound even doing.

Don't mix concatenation and string.Format it kills a puppy.

    private decimal _Total_tax;
    public decimal Total_tax
    {
        get
        {
            return this._Total_tax;
        }

        set
        {
            this._Total_tax = value + Total_tax;
        }
    }

Fields should be camel case. Either: totalTax or _totalTax.

A setter that keeps a running total isn't a good idea. Consider my example:

 TotalTax = 10;
 TotalTax = 15;
 TotalTax == 15; // False!?!?!?!?

You don't need to write:

if (is_import == true) {

You can just write:

if (is_import) {

Break the underscore key off of your keyboard. Just use camelCase for variables.

Edit

I think one of the other reasons for saying it isn't very OO is the fact that your CalcTax method (should be CalculateTax) takes a list of parameters when it would be better served by taking a Product object.

Answer 3

The company was likely looking for code that demonstrates object-oriented modeling. You have Product objects, but the description also mentions Taxes and Receipts. A more complete model might look like the following:

public interface Product
{
    string Description { get; }
    decimal Price { get; }
    bool IsImport { get; }
    bool IsFoodBookOrMedical { get; }
}

public interface ReceiptItem
{
    Product Product { get; }
    decimal Quantity { get; }
}

public interface ProductTax
{
    decimal Rate { get; }
    string Description { get; }
    bool IsApplicableTo(Product product);
}

public interface ReceiptCalculator
{
    IList<ProductTax> Taxes { get; } // List of taxes that need to be considered in the calculation
    IList<ReceiptItem> Items { get; }

    decimal TotalBeforeTax();
    decimal TotalTax();
    decimal Total();
}

Importantly (other reviewers/commenters also take note): in this model, ProductTax objects are responsible for deciding if they are applicable to a given product, and Products have no knowledge about Taxes. Adding/removing taxes from the calculation becomes possible through ReceiptCalculator's Taxes property, and different ReceiptCalculators could be created with different sets of tax considerations (e.g. for different states or countries).

Creating a taxonomy of Products based on tax classifications is not a good approach. If flag properties like IsImport and IsBookFoodOrMedical change frequently or grow to be too numerous, it would be better to generalize those into an ISet<...> property of some flag type. In a real system you don't want to have to make a change to your class hierarchy every time a tax is introduced or changed; you'd want to handle that by changing data. If a new tax is introduced that requires a different classification, all you have to do is tag the products that are applicable and create a ProductTax instance to look for it. (Implementing this would be worthwhile exercise.)

Answer 4

I'm going to be brutally honest, because I think it will do the most good for you as you look for a job. You say you "have the right frame of mind" and "just need someone to 'nudge' [you] in the right direction." You're way ahead of most of the pack, as you are capable of something much harder than FizzBuzz. However, this code needs a lot of work and a lot of reorganization. It's going to take time to learn the culture of C#, and it's going to take time to learn OO principles in an effective way. If I were you, I'd start writing a lot of OO code in C# in my free time and posting it to codereview.

This is C#. You don't need to call .Equals on strings. Instead, do:

if (men1 == "0")

You don't need ? true : false or the extra parentheses. Instead, do:

is_other = men1.Equals("2");

As everyone else said, you're not following C# naming conventions:

• For local variables, the first word is lower cased, and every word after that has its first letter capitalized. Underscores are not used. (e.g. isImport)
• Method names, class names, and property names all differ from locals by starting with a capital letter.

TotalTax shouldn't be a property on the program. Nothing is going to be accessing the Program file, so if it's going to be in there, it may as well just be an instance variable:

private decimal _TotalTax;

If you need to add to it....

_TotalTax += ...;

In if (item.count > 1), you do a lot of the same as in the else. Also, Console.WriteLine has an overload for a format string. Use that one instead of concatenating a bunch of strings and variables together. Also, it's hard to read when it's all on one line:

Console.Write("{0}: {1:0.00}",
    item.prod_name,
    CalcTax(item.sum, item.is_other, item.is_import));

if (item.count > 1)
    Console.Write(" ({0} @ {1})", item.count, item.prod_price);

Console.WriteLine();

CalcTax returns the price plus the tax. If it's going to do that, I would name it AddTax. There's also a lot of repetition in the method. Find out the part that's not repetitive, and do that first:

public decimal AddTax(decimal price, bool is_other, bool is_import)
{
    int percentage = 0;
    if (is_other) percentage += 10;
    if (is_import) percentage += 5;

    var tax = price * percentage / 100;
    Total_Tax = tax; //have to because of the value + in the property

    return price + tax;
}

I removed customRound because I don't think it's necessary.

I would also check for malformed input, as Convert.ToDecimal can fail with an error, and men1 can be something other than 0, 1, and 2. Before I got into the industry, I had flaky UIs that worked if you used it how I, the programmer, intended. There were quite a few ways to misuse it and break the program. One of the first things I learned on the job was that that was not going to fly.

Ewan's answer is great for the OO principles, so I'll neglect to cover that.

Answer 5

Using objects is not the same as doing object-oriented programming. Your code is very procedural and does not take advantage of the inherent flexibility of object-oriented design. The main concept you should be applying here is that you should encapsulate what varies. There are two approaches to accomplishing this that you could take: decoration or injection.

---

Decoration

We can approach this by looking at this from a decoration viewpoint. We have a base object that we are then layering on additional behavior to. A great pattern to use to layer additional behavior onto a base object is the Decorator Pattern. We can break each concept down and encapsulate it into an object representing that concept. (My C# may not be completely idiomatic, but it should work and exemplify the concept). First, we need a product:

public class Product
{
    public string Name { get; private set; }
    protected decimal Price { get; private set; }

    public Product(string name, decimal price) {
        Name = name;
        Price = price;
    }

    public decimal GetPrice() {
      return Price;
    }

    // Since we are taxing things, we probably would want the original price too.
    public decimal GetBasePrice() {
        return Price;
    }
}

Now to use the Decorator pattern, we need to extract an interface:

public class IProduct
{
    string Name { get; }
    decimal GetPrice();
    decimal GetBasePrice();
}

We add the interface to the Product class:

class Product : IProduct { ...

We now have encapsulated the idea of a product. We have left out anything to do with taxes or import fees because those are completely separate concepts that vary independently. Since we are encapsulating what varies, we know they should be separated. Now, let's create a decorator for a Product that is subject to tax:

class TaxableProduct : IProduct
{
    const decimal TAX_RATE = 0.10m;

    public string Name { get { return BaseProduct.Name; } }
    private IProduct BaseProduct { get; set; }

    public TaxableProduct(IProduct baseProduct) {
        BaseProduct = baseProduct;
    }

    public decimal GetPrice() {
        return BaseProduct.GetPrice() + BaseProduct.GetBasePrice() * TAX_RATE;
    }

    public decimal GetBasePrice() {
        return BaseProduct.GetBasePrice();
    }
}

We can do the same to create the ImportedProduct:

class ImportedProduct : IProduct
{
    const IMPORT_FEE = 0.05m;

    public string Name { get { BaseProduct.Name } }
    private IProduct BaseProduct { get; set; }

    public ImportedProduct(IProduct baseProduct) {
        BaseProduct = baseProduct;
    }

    public decimal GetPrice() {
        return BaseProduct.getPrice() + BaseProduct.GetBasePrice() * IMPORT_FEE);
    }

    public decimal GetBasePrice() {
        return BaseProduct.GetBasePrice();
    }
}

Now we can create an untaxed, unimported product:

new Product("Milk", 2.50m);

A new taxed, unimported product:

new TaxedProduct(new Product("Wrench", 8.00m));

A new imported, untaxed product:

new ImportedProduct(new Product("French cheese", 25.00m));

A new taxed, imported product:

new ImportedProduct(new TaxedProduct(new Product("Ferrari", 250000.00m)));

We are able to compose together the product with its tax and import fee calculations at runtime, while making it transparent through the IProduct interface. We have encapsulated the varying behavior of taxability and import-status into their own objects, and they can vary independently. If another type of fee comes along, we can just add a new decorator to encapsulate it. (BTW, there is duplication in this code that can easily be factored out.)

---

Injection

We can approach this by looking at this from an injection viewpoint. We have an object with some behavior, and we want to push more behavior into it. We can use Dependency Injection to push that behavior into the object. We can break each concept down and encapsulate it into an object representing that concept (My C# may not be completely idiomatic, but it should work and exemplify the concept). First, we need a product:

class Product
{
    public string Name { get; private set; }
    private decimal Price { get; set; }

    public Product(string name, decimal price) {
        Name = name;
        Price = price;
    }

    public decimal GetPrice() {
        return Price;
    }
}

Now a tax and an import fee are both fees on a product, so let's create an interface to encapsulate the concept of a fee:

interface IFee
{
    decimal CalculateFee(decimal price);
}

Now let's create our two fee implementations, TaxFee:

class TaxFee : IFee
{
    const decimal TAX_RATE = 0.10m;

    public decimal CalculateFee(decimal price) {
        return price * TAX_RATE;
    }
}

And ImportFee:

class ImportFee : IFee
{
    const decimal IMPORT_FEE = 0.05m;

    public decimal CalculateFee(decimal price) {
        return price * IMPORT_FEE;
    }
}

We want to inject our behavior into the product class, and the easiest way to do this is to pass parameters via the constructor. Since we want to pass an arbitrary number of fees, we can pass an IEnumerable<IFee>:

class Product
{
    public string Name { get; private set; }
    private decimal Price { get; set; }
    private IEnumerable<IFee> Fees { get; set; }

    public Product(string name, decimal price) : this(name, price, new List<IFee>()) {
    }

    public Product(string name, decimal price, IEnumerable<IFee> fees) {
        Name = name;
        Price = price;
        Fees = fees;
    }

    public decimal GetPrice() {
        decimal fullPrice = Price;

        foreach(IFee fee in Fees) {
            fullPrice += fee.CalculateFee(Price);
        }

        return fullPrice;
    }
}

Now we can create an untaxed, unimported product:

new Product("Milk", 2.50m);

A new taxed, unimported product:

List<IFee> fees = new List<IFee>();
fees.Add(new TaxFee());
new Product("Wrench", 8.00m, fees);

A new imported, untaxed product:

List<IFee> fees = new List<IFee>();
fees.Add(new ImportFee());
new Product("French cheese", 25.00m, fees);

A new taxed, imported product:

List<IFee> fees = new List<IFee>();
fees.Add(new TaxFee());
fees.Add(new ImportFee());
new Product("Ferrari", 250000.00m, fees);

We are able to compose together the product with its tax and import fee calculations at runtime, while making it transparent through the IFee interface. We have encapsulated the varying behavior of taxability and import-status into their own objects, and they can vary independently. If another type of fee comes along, we can just add a new IFee to encapsulate it. (BTW, there is duplication in this code that can easily be factored out.)

Answer 6

Too Many Trees in the answers

There are lots of valid points throughout the various answers but it mostly misses the big OO forest.

How could this be written to follow OOP concepts?

Create coherent classes.

A coherent class has appropriate properties to describe/define what it is and appropriate methods to describe/define what it does. This design goal is in play regardless of specific overall implementation.

The OP static void Main() does things that Product should do for itself: emit it's property values - ToString(); and CalculateTax().

Minimize coupling and maximize coherence

---

So, why is the code not object oriented?

Unorganized / Unstructured code

I want methods to be "at the appropriate level of abstraction." main should read very high level.

public static void main() {
    bool continueEntry = true;
    int userEntry = 0;

    while(continueEntry) {
       continueEntry = PrompUser(out userEntry);

       if(!continueEntry) break;

       CreateProduct(userEntry);
     }

     CalculateTax();
     PrintResults();
}

Anemic Classes

• Client code should not be doing things for a class.
  • tell Product to calculate tax. Don't do it for the product.
  • tell Product to display its details, Don't do it for the product.
  • Tell ProductList to calculate tax. Don't do it for the list.
  • Tell ProductList to display its details. Don't do it for the product.

Poor domain space modeling - Data Structures

The "items" are tax categories. We really don't care what it is but rather what its tax rate is.

I think of not just the classic things like trees, linked list, etc. but I tend to think of any class I create to make data (class properties) easier to manipulate.

Data Structures significantly reduces code complexity.

• Tax Rate Dictionary - Each taxable-item-type will have an entry. And the taxable-item-type will be the dictionary key.
• taxable-item-type - an enum
  • it is obvious that each taxable-item has it's own tax rate. Even if it is the same as other items.
• ProductList - This guy will know how to print the list, how to calculate total tax for the list, and how to find a duplicate inside of itself. Again, Tell [your class here] to do it, Don't do it for the [your class here]

ItemType

public enum ItemType {Invalid, Book, Food, Medical, Import, Other}

It is very, very useful to have a default of "invalid" or "unknown" or "undefined"

Tax Table

public class TaxTable {
    public static Dictionary<ItemType, decimal> Taxes = new Dictionary<ItemType, decimal>(){
    {ItemType.Invalid, 0},
    {ItemType.Book, 0},
    {ItemType.Food, 0},
    {ItemType.Medical, 0},
    {ItemType.Import, .05},
    {ItemType.Other, .10}
   }

   public static decimal GetTaxFor(ItemType) { // lookup in Taxes}
}

---

---

Product Class With Appropriate Responsibilities

I don't want to get wrapped around the axle about public, private, naming conventions, etc. That is irrelevant for the larger point of Object Oriented.

public class Product {
   ItemType WhatIAm {get; set;}

   // EDIT: Add "imported" as a separate concept. See @cbojar comment
   public bool Imported {get; protected set;}

   // a product knows what a valid item code is
   public Product(int itemType) {
       // convert to an ItemType

       // even if it is "Invalid" you can still have an Invalid
       // product type with a tax of zero, price of zero, etc.
       // Client code will be none the wiser.
       // The ProductCollection could simply elect to ignore it.
   }

   public override ToString(){
       StringBuilder me = new StringBuilder();
       me.AppendFormat("WhatIAm : {0}", WhatIAm);
       me.AppendFormat("Price : {0}", Price);
       // etc.
       return me.ToString();
   }

   // EDIT: added "imported" as an additional tax. See @cbojar comment
   public decimal CalculateTax() {
       decimal tax = 0.0;
       tax = Price * TaxTable.GetTaxFor(WhatIAm);
       tax = Imported ? tax + Price * TaxTable.GetTaxFor(ItemType.Imported) : tax;
   }

   public decimal TotalCost(){
       return Price + CalculateTax();
   }
}

ProductList With Appropriate Responsibilities

public class ProductList : List<Product> {

    public override ToString() {
        StringBuilder me = new StringBuilder();

        foreach(Product item in this) {
            me.AppendLine(item.ToString());
         }

         me.AppendFormat("Total Tax : {0}", CalcuateTax());
         return me.ToString();
    }

    public decimal CalculateTax(){
        decimal total = 0.0;

        foreach(Product item in this){
            total += item.CalculateTax();
        }

        return total;
    }
}

Client Code

THIS is object oriented

class Program {
    ProductList myProductList = new ProductList();

    public static void main() { 
        bool continueEntry = true;
        int userEntry = 0;

        while (continueEntry) {
           continueEntry = PrompUser(out userEntry);

           if(!continueEntry) break;

           CreateProduct(userEntry);
         }

         CalculateTax();
         PrintResults(); 
    }

    // yes, needs a Price parameter.
    public static void CreateProduct(int thisType) {
        myProductList.Add(new Product(thisType));
    }

    public static void CalculateTax() {
        myProductList.CalculateTax();
    }

    public string PrintResults(){
        Console.WriteLine(myProductList.ToString());
    }
}

Object Oriented Good Code Smells

• Client tells the class to do something.
• Code reads at appropriate levels of abstraction.
• Methods tend to be short!
  • Details are "pushed down" into the appropriate class.

• Domain parts are defined and distinct

  • Data Structures. Learn it, live it, love it.

• Unnecessary coupling is avoided

  • Anyone can use the TaxTable and ItemType
  • Each class does for itself. We are not dependent on another class to do it.
• code is reusable.
  • Good class responsibilites inherently makes Product (and in particular calculating product's tax) reusable.
• Class changes have no side effect, ideally.
  • Adding or changing the TaxTable does not cause a change in any other class
  • Changing Product.ToString() does not cause ProductList.ToString() to change.
• Treating objects the same.
  • There is no special case for an ItemType.Invalid product. Client code won't know or care. ProductList could not add invalid products to the list of you so desired, but even then the client is blissfully ignorant.

One final word... Not just on this thread, but everywhere there is way too much over emphasis on creating interfaces.

Answer 7

"it was not object orientated" is a weird response. It would make me instantly worried about the company as it suggests they are living in the past, so it's not somewhere I'd want to work.

Having said that, I'd imagine they were looking for something like you declaring an interface and classes to handle each tax type. Something like:

public interface IProduct
{
    string Name { get; set; }
    decimal Price { get; set; }
    decimal Tax { get; }
    bool Import { set; }
}

public class BookFoodOrMedical : IProduct
{
    public string Name { get; set; }
    public decimal Price { get; set; }

    public decimal Tax
    {
        get { return customRound(Price * (Import ? 5 : 0) / 100); }
    }

    public bool Import { set; private get; }
}

public class Other : IProduct
{
    public string Name { get; set; }
    public decimal Price { get; set; }

    public decimal Tax
    {
        get { return customRound(Price * (10 + (Import ? 5 : 0)) / 100); }
    }

    public bool Import { set; private get; }
}

Then, your menu method changes at the end to become:

if (is_other)
{
    prod_list.Add(new Other { Import = is_import, Name = prod_name, Price = prod_price });
}
else
{
    prod_list.Add(new BookFoodOrMedical { Import = is_import, Name = prod_name, Price = prod_price });
}

And the CalcTaxmethod becomes:

    public decimal CalcTax(IProduct product)
    {
        return product.Price + product.Tax;
    }

Whether this is better than what you wrote is moot though. Your solution works. By making it "more object orientated", I've arguably made the code harder to maintain. So whilst I've shown how this could written to follow OOP concepts, you are better off sticking to your approach and simply following the advice in other answers on how to better follow C# conventions and how to improve the code.

Answer 8

Step 1 in this type of scenario is to identify the actors and behaviors in the description. We are not making a stock exchange which monitors companies values with relation to their itemized finances every nanosecond, we are only making exactly what is being described.

Write a program the feeds in prices. Actors? Behaviors? Prices being fed somewhere is a behavior, so no actors yet. Unfortunately, the word "program" isn't clarified with the type of program, so let's call this a register program, and that is where the prices will be fed in to.

If the item is a Book, Food or Medical product there is no tax, if not there is a 10% tax (other item). Actors? Behaviors? There are four actors here, Book, Food, Medical, Other. They each have a tax behavior. They are all products. We previously know that we are also dealing with prices from these actors.

If the item is an import there is a 5% tax (it's possible to have an import and "other item" type of item with 15% tax (10 + 5)). Actors? Behaviors? There is the behavior of an import for a product. This is a modification of an already existing actor.

The program needs to print receipts Actors? Behaviors? This goes back to not having a name for a program, so I will assume program is my Register class again. Print is an action and receipt shows a format with a name attached.

This is the end of parsing the requirements. So far we have the requirement of a register class, a class for each product type, a base class to share the behaviors, and an enum for tax modification. The register class should be able to calculate the prices and print. Let's break this out a little.

The enum for taxes and the base product class

public enum TaxModificationPercent
{
    Import  = 5
}

public abstract class Product
{
    public decimal Price { get; protected set; }
    public string Name { get; protected set; }
    protected decimal _Tax = 0;
    public virtual decimal Tax 
    { 
        get { return _Tax; } 
        protected set { _Tax = value; }
    }

    public Product(string Name, decimal Price)
    {
        this.Name = Name;
        this.Price = Price;
    }

    public Product(string Name, decimal Price, TaxModificationPercent taxMod) 
        : this(Name,Price)
    {
        this.Tax += (decimal)taxMod/100;
    }
}

The set of product classes

public class Book : Product
{
    public Book(string Name, decimal Price) : base(Name, Price){}
    public Book(string Name, decimal Price,TaxModificationPercent taxMod) 
        : base (Name, Price, taxMod){}
}

public class Food : Product
{
    public Food(string Name, decimal Price) : base(Name, Price){}
    public Food(string Name, decimal Price,TaxModificationPercent taxMod) 
        : base (Name, Price, taxMod){}
}

public class Medical : Product
{
    public Medical(string Name, decimal Price) : base(Name, Price){}
    public Medical(string Name, decimal Price,TaxModificationPercent taxMod) 
        : base (Name, Price, taxMod){}
}

public class Other : Product
{
    private decimal _tax = 0.1M;
    public override decimal Tax 
    { 
        get { return _tax; } 
        protected set { _tax = value; }
    }

    public Other(string Name, decimal Price) : base(Name, Price){}

    public Other(string Name, decimal Price,TaxModificationPercent taxMod) 
        : base (Name, Price, taxMod){}
}

And the register class which will hold the products and print receipts

public class Register
{
    private List<Product> products = new List<Product>();

    public void Feed(Product product)
    {
        this.products.Add(product);
    }

    public void Print()
    {
        decimal total = 0;
        foreach(var nameGroup in this.products.GroupBy( p => p.Name ))
        {
            decimal item = nameGroup.Sum( p => p.Price );
            total += item;
            StringBuilder lineItem = new StringBuilder();
            lineItem
                .Append(nameGroup.Key)
                .Append(": " )
                .Append(string.Format("{0:C}",item));
            if( nameGroup.Count() > 1 ) 
                lineItem.Append(" (")
                    .Append(nameGroup.Count())
                    .Append(" @ ")
                    .Append(string.Format("{0:C}",nameGroup.First().Price))
                    .Append(")");
            Console.WriteLine(lineItem.ToString());
        }
        decimal tax = this.products.Sum( p => p.Price*p.Tax );
        Console.WriteLine("Sales Taxes: " + string.Format("{0:C}",tax));
        Console.WriteLine("Total: " + string.Format("{0:C}",total));
    }
}

Finally, the use for this would be to simply add new instances of these items

var register = new Register();

register.Feed(new Book("Robinson Crusoe",4.56M));
register.Feed(new Book("Robinson Crusoe",4.56M));
register.Feed(new Food("Imported dog food",6.52M,TaxModificationPercent.Import));//Was uncertain if this was Food or Other
register.Feed(new Other("Perfume",6.23M));

register.Print();

Here is a demo: https://dotnetfiddle.net/gCTYOt
back to top

Answer 9

In the text several categories are described. OOD would try and capture those categories as classes. There's the categories of imported goods and goods that are not imported. The there's the categories of tax exempted goods and goods with tax. If you are only ever gonna have one perfurme or one dog food this approach is over kill. The main idea is to show how to codify classifications even when they intersect

You can use it like this

var book = new Product("book", 4.56m);

or for those that you can have in multiple variations you can subclass

class DogFood : TaxedProduct{...}

var dogFood = new DogFood(...);
var importedDogFood = new Imported<DogFood>(...);

The class hierachi could look something like this

public abstract class Product
{
    public string Name { get; protected set; }
    public decimal Price { get; protected set; }

    public abstract decimal Taxes
    {
        get;
    }
}


public class TaxedProduct : Product
{
    public Taxed(string name, decimal price){
        Name = name;
        Price = price;
    }
    public Taxed(){}
    public override decimal Taxes
    {
        get
        {
            return Price * .1m;
        }
    }
}

public class Imported<TProduct> : Product where TProduct : Product, new()
{
    TProduct _base = new TProduct();
    public Imported(string name, decimal price) {
         Price = _base.Price = price;
         Name = _base.Name = name;
    }
    public override decimal Taxes
    {
        get
        {
            return Price * .05m + _base.Taxes;
        }
    }
}

Answer 10

Refactoring

I took the code you had and spent about 10 minutes just refactoring it as I believe that is a good way to get to OOD and OOP. Refactoring is simply rearrangement of the code (to start). The idea is to make is easy to understand.

Here's an example of a refactor (below). Couple of things about VS2013 refactoring that I don't care for 1) When you generate methods they come up static sometimes when you don't want that 2) If you have global vars, and you generate a method it will name the input parm the same thing. I didn't change those two things to show you what happens.

namespace Cash
{
 public class product
  {
    public bool is_import { get; set; }

    public bool is_other { get; set; }

    public string prod_name { get; set; }

    public decimal prod_price { get; set; }
}

I moved the vars to global scope, as it makes sense, but is a preference only. I do this myself to remind of the need to initialize the content and to ensure they are always re-inited later.

internal class Program
{
    private decimal _Total_tax = 0;

    private bool isImport = false;

    private bool isOther = false;

    private List<product> productList = new List<product>();

    private string productName = string.Empty;

    private decimal productPrice = 0;

    private string selectedMenuItem = string.Empty;

    public decimal Total_tax
    {
        get
        {
            return this._Total_tax;
        }

        set
        {
            this._Total_tax = value + Total_tax;
        }
    }

This section has many repeating patterns. Each if statement calls customRound (twice), when you see repeating patterns you can think about abstracting them.

One thing that comes to mind is passing in a product, or a superclass of product with a tax field. All products have prices and all products will have a tax even if it's zero. Taxes are not usually a part of the product itself but could be a decoration of the product.

public decimal CalcTax(decimal price, bool is_other, bool is_import)
{
    decimal _price = 0;

    if (is_import == true && is_other == true)
    {
        _price = customRound((price * 15) / 100) + price;
        Total_tax = customRound((_price - price));
    }

    if (is_import == true && is_other == false)//
    {
        _price = customRound((price * 5) / 100) + price;
        Total_tax = customRound((_price - price));
    }

    if (is_import == false && is_other == true)
    {
        _price = customRound((price * 10) / 100) + price;
        Total_tax = customRound((_price - price));
    }

    if (is_import == false && is_other == false)
    {
        _price = price;
        Total_tax = customRound((_price - price));
    }

    return _price;
}

public decimal customRound(decimal num)
{
    return Math.Round(num * 20.0M, MidpointRounding.AwayFromZero) / 20.0M;
}

public void Print(List<product> prod_list)
{
    var result = prod_list.GroupBy(x => new { x.prod_name, x.prod_price, x.is_other, x.is_import })
                                     .Select(x => new
                                     {
                                         prod_name = x.Key.prod_name,
                                         sum = x.Sum(z => z.prod_price),
                                         count = x.Count(),
                                         prod_price = x.Key.prod_price,
                                         is_other = x.Key.is_other,
                                         is_import = x.Key.is_import,
                                     });

    foreach (var item in result)
    {
        if (item.count > 1)
        {
            Console.WriteLine(item.prod_name + ": " + string.Format("{0:0.00}", CalcTax(item.sum, item.is_other, item.is_import)) + " (" + item.count + " @ " + item.prod_price + ")");
        }
        else
        {
            Console.WriteLine(item.prod_name + ": " + string.Format("{0:0.00}", CalcTax(item.sum, item.is_other, item.is_import)));
        }
    }

    Console.WriteLine("Sales Taxes: " + string.Format("{0:0.00}", Total_tax));
    Console.WriteLine("Total: " + string.Format("{0:0.00}", prod_list.Sum(item => item.prod_price) + Total_tax));
    Console.ReadLine();
}

This is the main entry point and now looks more like a controller than a large bunch of code.

public void ShowMenu()
{
    while (true)
    {
        Initialize();

        selectedMenuItem = GetAnswers(selectedMenuItem);

        if (UserSelectedEnd()) break;

        isImport = CheckImport(isImport);

        isOther = CheckOther(selectedMenuItem, isOther);

        productPrice = EnterPrice(productPrice);

        productName = EnterProductName(productName);

        AddProduct(productName, isImport, isOther, productPrice, productList);
    }
}

This is how refactoring would have clued me into the need to pass product around:

public void ShowMenu()
{
    while (true)
    {
        Initialize();

        selectedMenuItem = GetAnswers(selectedMenuItem);

        if (UserSelectedEnd()) break;

        var product = new product();

        GetImportAnswer(product);

        GetOtherAnswer(product);

        GetPriceAnswer(product);

        GetProductName(product);

        productList.Add(product);
    }
}

These are the auto generated methods from Visual Studio Note they are all Static (not necessarily a good thing). Refactoring into small chunks like this allows you to start thinking about separation of concerns. Over time you'll automatically do this because it's much easier to understand.

private static void AddProduct(string productName, bool isImport, bool isOther, decimal productPrice, List<product> productList)
{
    productList.Add(new product { is_import = isImport, prod_name = productName, prod_price = productPrice, is_other = isOther });
}

Note: Each of these methods below could have taken a product instance in as a parameter. I find it is often much better to pass class instances when dealing with lots of properties. This would be a redesign of your program where a new product is instantiated at top of question loop.

private static bool CheckImport(bool isImport)
{
    Console.WriteLine("Is this product an import? (Y/N)");

    isImport = (Console.ReadLine().ToUpper().Equals("Y")) ? true : false;
    return isImport;
}

private static bool CheckOther(string selectedMenuItem, bool isOther)
{
    isOther = (selectedMenuItem.Equals("2")) ? true : false;
    return isOther;
}

private static decimal EnterPrice(decimal productPrice)
{
    Console.WriteLine("***Please enter the price***");
    productPrice = Convert.ToDecimal(Console.ReadLine());
    return productPrice;
}

private static string EnterProductName(string productName)
{
    Console.WriteLine("Please enter the descriptive name of the product");
    productName = Console.ReadLine();
    return productName;
}

private static string GetAnswers(string selectedMenuItem)
{
    Console.WriteLine("Sales Tax");
    Console.WriteLine();
    Console.WriteLine("***Please Select the Item Type***");
    Console.WriteLine("1. Book, Food or Medical Product");
    Console.WriteLine("2. Other");
    Console.WriteLine("Please select a menu option or 0 to end:");
    selectedMenuItem = Console.ReadLine();
    return selectedMenuItem;
}

If I wanted to use the product as a parameter, the code above would look like this:

private void GetImportAnswer(product prod)
{
    Console.WriteLine("Is this product an import? (Y/N)");

    prod.is_import = (Console.ReadLine().ToUpper().Equals("Y")) ? true : false;
}

private void GetOtherAnswer(product prod)
{
    prod.is_other = (selectedMenuItem.Equals("2")) ? true : false;
}

private void GetPriceAnswer(product prod)
{
    Console.WriteLine("***Please enter the price***");
    prod.prod_price = Convert.ToDecimal(Console.ReadLine());
}

private void GetProductName(product prod)
{
    Console.WriteLine("Please enter the descriptive name of the product");
    prod.prod_name = Console.ReadLine();
}

private  string GetAnswers(string selectedMenuItem)
{
    Console.WriteLine("Sales Tax");
    Console.WriteLine();
    Console.WriteLine("***Please Select the Item Type***");
    Console.WriteLine("1. Book, Food or Medical Product");
    Console.WriteLine("2. Other");
    Console.WriteLine("Please select a menu option or 0 to end:");
    selectedMenuItem = Console.ReadLine();
    return selectedMenuItem;
}
private static void Main(string[] args)
{
    Program prog = new Program();
    prog.ShowMenu();
}
private void Initialize()
{
    selectedMenuItem = null;
    productName = string.Empty;
    isImport = false;
    isOther = false;
    productPrice = 0;
}
private bool UserSelectedEnd()
{
    if (selectedMenuItem.Equals("0"))
    {
        Print(productList);
        return true;
    }
    return false;
}

Anyway, I thought you showed some good C# skills. You'll find a good job!

Answer 11

From the problem statement,

we could lay out classes like below:

class Product {
    private String name;
    private doublt price;
    //Getter setter, constructor etc
}
class Book extends Product implements NonTaxable { ... }
class Food extends Product implements NonTaxable { ... }

A product can be:

interface Taxable {
    float taxRate();
}

Or, it can also be (a special case)

interface NonTaxable extends Taxable {
    default float taxRate(){
        return 0.0f;
    }
}

One must be able to tell which Product by looking at the input text:

interface ProductFactory {
    Taxable parseProduct(String desc);
}

We have some 'extra' calculation for a special case:

class ImportedProduct extends Product implements Taxable {
    private Taxable product;
    public ImportedProduct(Taxable taxableProduct){
        this.product = taxableProduct;
    }

    float taxRate(){
        return .05 + this.product.taxRate();
    }
}