23
\$\begingroup\$

I applied for a Junior .NET Developer Position recently, and I was asked to solve a problem (Sales Tax Program) and I was rejected for some reason. The question goes like this:

Basic sales tax is applicable at a rate of 10% on all goods, except food, medicines and books. For all imported goods, a tax rate of additional 5% is applied irrespective of their type. Generate a receipt which shows names of all the items purchased and their prices (including taxes), finishing with total cost of the items, and total amount sales tax paid. The rounding rules for sales tax are that for a tax rate of n%, a shelf price of P contains(np/100 rounded up to nearest 0.05) amount of sales tax. Write an application to generate receipt with all these details.

They gave me the input to take and output should come:

Input 3:

  • 1 imported bottle of perfume at 27.99
  • 1 bottle of perfume at 18.99
  • 1 packet of headache pills at 9.75
  • 1 box of imported chocolates at 11.25

Output 3:

  • 1 imported bottle of perfume: 32.19
  • 1 bottle of perfume: 20.89
  • 1 packet of headache pills: 9.75
  • 1 imported box of chocolates: 11.85
  • Sales Taxes: 6.70
  • Total: 74.68

They gave two more sets of input, but I just didn't put that to save some time.

Please review my code and let me know what's wrong with that and what are the concepts I need to focus on to solve a problem like this.

namespace Products
{
    class Program
    {
        static void Main(string[] args)
        {
            try
            {
                // calling the Generatereciept class and invoking Generate method to display all the product details, price, taxes and total after taxes
                GenerateReciept generate = new GenerateReciept();
                generate.Generate();
            }
            catch (Exception ex)
            {
                // writing the exception to console.. we can log it to a database or eventviewer based on the requirements
                Console.WriteLine(ex.Message);

            }

            Console.ReadLine();
        }
    }
    /// <summary>
    /// enum for determining whether a product is imported or not
    /// </summary>
    public enum ProdType
    {
        Imported = 1,
        NonImported = 2
    }
    /// <summary>
    /// enum for determining whether product is tax exempted or not
    /// </summary>
    public enum TaxStatus
    {
        Taxable = 1,
        NonTaxable = 2
    }
/// <summary>
/// contains the product details
/// </summary>
class Product
{
    private string _Name;
    private TaxStatus _TaxStatus;
    private decimal _Price;
    private ProdType _Typeofproduct;

    public string Name
    {
        get
        {
            return _Name;
        }
    }

    public TaxStatus TaxStatus
    {
        get
        {
            return _TaxStatus;
        }
    }
    public decimal Price
    {
        get
        {
            return _Price;
        }
    }
    public ProdType Typeofproduct
    {
        get
        {
            return _Typeofproduct;
        }
    }

    /// <summary>
    /// Initializes the fields
    /// </summary>
    /// <param name="name"></param>
    /// <param name="taxStatus"></param>
    /// <param name="price"></param>
    /// <param name="typeOfProduct"></param>
    public Product(string name, TaxStatus taxStatus, decimal price, ProdType typeOfProduct)
    {
        _Name = name;
        _TaxStatus = taxStatus;
        _Price = price;
        _Typeofproduct = typeOfProduct;
    }
}

/// <summary>
/// maintains product list
/// </summary>
class ProductsList
{
    /// <summary>
    /// Based on the input given, added products and their respective info to the list
    /// </summary>
    /// <returns></returns>
    public List<Product> GetProducts()
    {
        List<Product> list = new List<Product>
       {
           //input 1
           new Product("Book", TaxStatus.NonTaxable, 12.49M, ProdType.NonImported),
           new Product("MusicCD", TaxStatus.Taxable, 14.99M, ProdType.NonImported),
           new Product("ChocolateBar", TaxStatus.NonTaxable, 0.85M, ProdType.NonImported),

           ////input 2
           //new Product("Imported  box of Chocolates", TaxStatus.NonTaxable, 10.00M, ProdType.Imported),
           //new Product("Imported bottle of Perfume", TaxStatus.Taxable, 47.50M, ProdType.Imported),

           ////input 3
           //new Product("Imported bottle of Perfume", TaxStatus.Taxable, 27.99M, ProdType.Imported), 
           //new Product("Bottle of Perfume", TaxStatus.Taxable, 18.99M, ProdType.NonImported), 
           //new Product("Packet of headache Pills", TaxStatus.NonTaxable, 9.75M, ProdType.NonImported), 
           //new Product("Box of Imported Chocolates", TaxStatus.NonTaxable, 11.25M, ProdType.Imported)
       };
        return list;
    }
}



 /// <summary>
    /// declares a method for calculating basic sales tax 
    /// </summary>
    interface IbasicTax
    {
        decimal CalculateBasicTax();
    }
    /// <summary>
    /// declares an method for calculating full tax( basic sales tax + additional tax for imported goods)
    /// </summary>
    interface IFullTax
    {
        decimal CalculateFullTax();
    }
    /// <summary>
    /// declares method for calculating imported tax only( non taxable imported products)
    /// </summary>
    interface IImportedTax
    {
        decimal CalculateAdditionalTax();
    }
    /// <summary>
    /// class inheriting from all the interfaces and implementing them
    /// </summary>
    class Tax : IFullTax, IbasicTax, IImportedTax
    {
        decimal tax = 0;
        decimal _price;

        public Tax(decimal price)
        {
            _price = price;
        }
        public decimal CalculateBasicTax()
        {
            return tax += 0.1M * _price;
        }

        public decimal CalculateAdditionalTax()
        {
            return tax += 0.05M * _price;
        }

        public decimal CalculateFullTax()
        {
            return tax += 0.15M * _price;
        }
    }

    class GenerateReciept
    {
        decimal TotalSum = 0, TotalTaxes = 0;

        /// <summary>
        /// Generates the products reciept based on the taxes imposed on the products... calculates and displays in an order suggested
        /// </summary>
        public void Generate()
        {
            // Program program = new Program();
            ProductsList list = new ProductsList();
            List<Product> products = list.GetProducts();

            foreach (Product p in products)
            {
                decimal a = 0;
                decimal Taxamount = 0;
                if (p.TaxStatus == TaxStatus.Taxable)
                {
                    if (p.Typeofproduct == ProdType.Imported)
                    {
                        IFullTax tax = new Tax(p.Price);
                        a = tax.CalculateFullTax();

                    }
                    else
                    {
                        IbasicTax tax = new Tax(p.Price);
                        a = tax.CalculateBasicTax();

                    }
                }
                else
                {
                    if (p.Typeofproduct == ProdType.Imported)
                    {
                        IImportedTax tax = new Tax(p.Price);
                        a = tax.CalculateAdditionalTax();

                    }
                }
                Taxamount = Math.Ceiling(a * 20) / 20;
                TotalTaxes += Taxamount;
                TotalSum += Taxamount + p.Price;

                Console.WriteLine("1 {0} : {1}", p.Name, Taxamount + p.Price);
            }

            Console.WriteLine("Total Sales Tax : {0}", TotalTaxes);
            Console.WriteLine("Total after taxes: {0}", TotalSum);

        }
    }
}

I have got an output but for some reason it was rejected. Please let me know what are the flaws in my Code. Even if it is completely flawed, please let me know. I want to correct myself and try not to repeat the same mistake again.

\$\endgroup\$
  • \$\begingroup\$ Do you get the expected output or something else? \$\endgroup\$ – Jeroen Vannevel May 20 '15 at 20:43
  • \$\begingroup\$ I got the output .. I got the ouput exactly what they asked me show. \$\endgroup\$ – Juggernaut May 20 '15 at 20:44
  • 3
    \$\begingroup\$ Similar question here. \$\endgroup\$ – mjolka May 20 '15 at 22:42
  • 5
    \$\begingroup\$ Also, nothing is stopping you from actually asking the interviewer(s) specific reasons for rejection. The worst they can do is decline or not respond. Otherwise, you'll find yourself with good information that hopefully jibes with the answers given. \$\endgroup\$ – Jesse C. Slicer May 21 '15 at 15:31
26
\$\begingroup\$

As an interviewer, I want to see candidates demonstrate the skills I'm hiring for. In particular, I want to see unit tests, because I want you to write tests for your code if you come work for me. If you had written unit tests for your code, you probably would have caught many of the items listed in the other answers. You at least would have demonstrated an organized approach to coding your solution.

It's hard to say what the interviewer may have been looking for, because we're not him. He might have wanted to see if you recognized when to apply the strategy pattern (because of the different types of items that have different taxation rules.) He might have been looking for you to organize the items around a transaction (not just a product list), passing the transaction first to a tax calculator, and then to a receipt generator, using Dependency Injection. Maybe he looked at the monolithic function of Generate() and said "too much complexity in a single method." Maybe he was looking for overall better evidence of the SOLID principles.

We obviously can't know exactly what he wanted. You demonstrated a grasp of the language, you just need to apply object-oriented design principles to your coding. I think that's something you can teach a junior developer. So maybe he was fishing for someone skilled but only wanted to pay a junior developer's wage.

\$\endgroup\$
  • 3
    \$\begingroup\$ you look for unit tests for a Junior position? I think my definition of a junior is very different for yours. Or is this just a general statement? \$\endgroup\$ – James Khoury May 21 '15 at 3:46
  • 7
    \$\begingroup\$ I expect unit tests from every single person who calls him- or herself a software engineer, regardless of level. How else do you prove to me that your logic works, unless your tests demonstrate it? How else do you prove to me that your code has a usable interface, unless you're able to write a test that actually uses it? Anyone can eventually stumble into some code whose output accidentally meets the requirements; that doesn't mean I can afford to maintain it. Without producing unit tests it's not software engineering. \$\endgroup\$ – John Deters May 21 '15 at 3:57
  • 10
    \$\begingroup\$ I just find it surprising that you'd expect it at a junior level. I didn't learn this coming out of University and this is the kind of stuff I had to learn on the job. I would expect a junior to have the ability to learn rather than the experience to know these things. Though it would definitely be a plus if the candidate could unit test I would see that as learn-able whereas if they could not show a basic concept of programming then their unit tests would be irrelevant. \$\endgroup\$ – James Khoury May 21 '15 at 4:04
  • 2
    \$\begingroup\$ @JamesKhoury Depends whether "Junior developer" means "Graduate developer" at this company. It may not. \$\endgroup\$ – Ben Aaronson May 21 '15 at 8:58
  • 3
    \$\begingroup\$ @BenAaronson "Junior developer" and "Graduate developer" need not be the same thing. Nevertheless, since "Junior developer" is by nature the entry level, any graduate developer will be a junior developer. \$\endgroup\$ – Jasper May 21 '15 at 9:01
13
\$\begingroup\$

Janos has some great pointers. There are other issues that stuck out quite a bit, from my experience, so I'll add mine:

Product-specific logic outside of Product Class

Your Generate() function has logic that seems specific to a Product, which contradicts the object-oriented nature of C#. The specific code I'm referring to is:

foreach (var p in products)
{
    decimal a = 0;
    decimal Taxamount = 0;
    if (p.TaxStatus == TaxStatus.Taxable)
    {
        if (p.Typeofproduct == ProdType.Imported)
        {
            IFullTax tax = new Tax(p.Price);
            a = tax.CalculateFullTax();
        }
        else
        {
            IbasicTax tax = new Tax(p.Price);
            a = tax.CalculateBasicTax();
        }
    }
    else
    {
        if (p.Typeofproduct == ProdType.Imported)
        {
            IImportedTax tax = new Tax(p.Price);
            a = tax.CalculateAdditionalTax();

        }
    }

    Taxamount = Math.Ceiling(a * 20) / 20;
    // ...
}

It would be a lot clearer to isolate the product logic into the class itself. For example, I would suggest initially refactoring the Generate() code to:

foreach (var p in products)
{
    decimal a = p.CalculateTax();
    decimal Taxamount = Math.Ceiling(a * 20) / 20;
    // ...
}

And then renaming variables to further clarify what's being calculated:

foreach (var p in products)
{
    decimal initialTaxAmount = p.CalculateTax();
    decimal roundedTaxAmount = Math.Ceiling(initialTaxAmount * 20) / 20;
    // ...
}

Interfaces

The way in which you used your interfaces don't serve any real purpose.

Also, if you're adding interfaces just for the sake of adding interfaces, often I've found that makes the code confusing to read. There's nothing wrong with keeping it simple.

public decimal CalculateTax()
{
    var tax = new Tax(price);

    if (condition1)
        return tax.CalculateFullTax();
    else if (condition2)
        return tax.CalculateBasicTax();
    else
        return tax.CalculateImportedTax();
}

Later, if you find a good case for using an interface in place of a specific class, refactor it then.

You can also refactor my function above so that it doesn't keep creating a new Tax object every time you try to calculate the tax. I was just using it as an example.

Multiple Items

I'm not sure if this is part of the requirements or not. That said, your code does not handle multiple items.

Generate() only prints out each item, one at a time. You even have the hard-coded output of 1 item:

Console.WriteLine("1 {0} : {1}", p.Name, Taxamount + p.Price);

Also, if you did want to add more than one item, FetchProducts() is not set up to return multiple of the same instance of Product. With your current code, it would seem natural to just add a new Book as:

public List<Product> GetProducts()
{
    List<Product> list = new List<Product>
   {
       new Product("Book", TaxStatus.NonTaxable, 12.49M, ProdType.NonImported),
       new Product("MusicCD", TaxStatus.Taxable, 14.99M, ProdType.NonImported),
       new Product("ChocolateBar", TaxStatus.NonTaxable, 0.85M, ProdType.NonImported),
       new Product("Book", TaxStatus.NonTaxable, 12.59M, ProdType.NonImported),
    }
    return list;
}

The problem with this is it increases the memory used and allows for different properties for the same item. Did you notice the 0.10 more for the second book?

Output Formatting

When you run your code, TotalTaxes prints as 1.5 instead of 1.50 If using Console.WriteLine, there are several numeric format strings to choose from. Given the format they have, this would work:

Console.WriteLine("Total Sales Tax : {0:N}", TotalTaxes);

ProductLists

Even though Janos mentioned this, I feel it should be reiterated. The ProductList class is not necessary, at least in its current state. It's a static list that doesn't take any input from the user, nor does it implement any interfaces that might suggest it could be attached to any sort of datastore containing an inventory of products.

My recommendation might have been some method to mimic an inventory database listing with a way to search for a specific item.

public MockInventory : IInventory
{
    private List<Product> allProducts;

    public MockInventory()
    {
        allProducts = new List<Product>
        { 
            new Product("Book", TaxStatus.NonTaxable, 12.49M, ProdType.NonImported),
            new Product("MusicCD", TaxStatus.Taxable, 14.99M, ProdType.NonImported),
            new Product("ChocolateBar", TaxStatus.NonTaxable, 0.85M, ProdType.NonImported),
            new Product("Imported  box of Chocolates", TaxStatus.NonTaxable, 10.00M, ProdType.Imported),
            new Product("Imported bottle of Perfume", TaxStatus.Taxable, 47.50M, ProdType.Imported),
            // Etc..
        };
    }

    // Interface method to search for a product
    public Product Find(string name)
    {
        // ... 
    }
}

Hope this helps.

\$\endgroup\$
  • \$\begingroup\$ Thanks for taking some time and explaining how things went wrong .. Yeah, I intentionally used interface because I was going for junior level and I wanted to show them that I can use inheritance chain and break down the problem into individual modules and server the purpose but was failed miserably. It would be very helpful for me if you can provide me any link or material or code which shows how to write a code in a proper way using SOLID principles and OOD .. Thank you so much \$\endgroup\$ – Juggernaut May 21 '15 at 13:50
11
\$\begingroup\$

Some serious problems:

  1. The Main method catching an Exception, and I can't find a reason why.

    • If you have to handle some exceptions, you should catch the most specific type possible that might be thrown.
    • If you don't expect exceptions to be thrown, don't catch
    • In the Main method, you add no value by catching exceptions only to print them to the console. In such situation you could declare Main to throw an exception, the runtime will print the stack trace anyway
  2. Interfaces with a single method are often fishy. I don't see much purpose for these interfaces and they are also poorly named. A single TaxCalculator interface with 3 methods would have been better. But looking at the rest of the program, I don't see much purpose for interfaces at all.

  3. The ProductsList class seems pointless. It doesn't encapsulate what its name suggests: doesn't contain a list of products and doesn't manage a list of products. It has a method that creates a list of products, but a class is not required for such functionality. I recommend to read about Abstract Data Types in the book Code Complete (chapter 6).

  4. The naming of variables and methods is quite poor and inconsistent throughout, making the code look messy overall.

These are smaller problems, but still not pretty:

  1. Why assign values in the enumerations? Don't write code that serves no purpose. You could drop those values without losing anything

  2. GetProducts creates a new list of objects every time you call it. Normally I wouldn't expect a method named GetSomething to allocate memory. If you renamed to CreateProducts, it would make it more intuitive. The local list variable is also redundant, it would be better to return a new list directly.

\$\endgroup\$
  • \$\begingroup\$ Thank you so much for taking some time and providing the inputs. It would be really helpful for me if you could suggest me some books which help me in writing clean code in C#. \$\endgroup\$ – Juggernaut May 20 '15 at 21:57
  • \$\begingroup\$ The reason for including interfaces is that I wanted to show them that I have knowledge of OOP, but it backfired. They asked me to show OOP in the code i write. So, I used interfaces to show inheritance. I used enums to just categorize the ProductType( Imported or not, Taxable or not) ..I did not get any idea how to solve that so I used enums. Any suggestion to achieve that would be appreciated. \$\endgroup\$ – Juggernaut May 20 '15 at 22:03
  • \$\begingroup\$ "If you don't expect exceptions to be thrown, don't catch" Hmm... On an example as trivial as this one, I'd agree with that, but for large, production systems, that's not necessarily the best advice. Logging stack traces to a file and displaying something a little more clean than the runtime's unhandled exception dialog will probably be more useful and not lower the user's opinion of your software quality quite as much. Specifying values for enums can also be useful in situations where you might ever need to interoperate with code in a different platform. \$\endgroup\$ – reirab May 21 '15 at 7:05
10
\$\begingroup\$

In my opinion, the biggest concern is that products are modelled with tax status as the most important property. This makes reusing the objects in different contexts, for example an inventory control system or an online storefront difficult. Furthermore, the tax status of a book is not a constant because in another jurisdiction, it may be taxable at the base rate and a book printed in Bolivia may be an imported book in Australia, but it is not in Bolivia.

The code may meet the specification, but it is extremely brittle. It can not easily modified to address related circumstances or support making logically implied business decisions because it does not strive to fit into the bigger context of running a business. Code is a means to an end, not an end in itself.

Modelling

The key to designing a well-engineered solution is to have an appropriate model of the system. Sure there are times when quick and dirty is good enough, but a core business function such as sales transactions are usually not among them.

Naive Model

The naive model is specified in terms of computer input and output. We get an order and produce a receipt.

Order to Receipt

It doesn't concern itself with how the transformation takes place or what the long term implications of a particular implementation might be.

Simple Business Centred Model

A receipt documents the exchange of goods for payment. So a sound business doesn't produce a receipt when an order is placed. The receipt is generated only after the sale of the ordered items.

Order to Sale to Receipt

A receipt is a record of a transaction not an actual transaction. And we need an intermediate place to actually conduct the transaction.

System Interfaces

Anywhere there is an arrow there is some sort of intermediate transformational process.

An Order is some sort of record that is suitable for the ordering process. It might tie into other systems in ways that aren't really relevant to the actual sales transaction. It's format may not be ideal for conducting the sale, so we need a step where we read an order and produce a data structure that is suitable for the sales process.

Order to Normalize to Sale

Likewise, the ideal data structure for a sale may not be ideal for a receipt. So there is another interface that formats the sales data into a data structure suited for creating receipts.

Sale to Format to Receipt

Course Grained Objects

At each stage: order, sale, receipt, there is an aggregate object.

Order Object to Sale Object to Receipt Object

It's worth noting that at this level of abstraction there's obvious isomorphism between the Order-Object and the Sale Object.

Fine Grained Objects

Each step of the process has a corresponding fine grained record type that captures the individual items of the aggregate.

Order Item

Sale Item

Receipt Item

Notice that at this level:

  1. The isomorphism is between the Order Item and the Receipt Item. This is consistent with the Naive Model of input output.
  2. The Sale Item captures the business logic in regard to taxes.
  3. The Receipt Item is where the aggregate properties of a sale are captured as totals. We don't necessarily need cache these as part of the Sale because they are computable from the Sale Object if someone else needs them. There are cases where they won't such as if there are early payment discounts that may modify the sale later.

Normalize

The process of normalizing consists of:

  1. Sorting the List of Order Item in the Order Object.
  2. Processing each Order Item.
    1. Breaking apart any Order Items where quantity > 1 into individual line items.
    2. Mapping Each Order Item into a Sale Item.

Format

The process of formatting consists of:

  1. Sorting the List of Sale Item in the Sale Object.
  2. Processing each Sale Item.
    1. Adding it's cost to a running total cost.
    2. Adding it's tax to a running total tax.
    3. Aggregating any Sale Items where quantity > 1 into a single line item.
    4. Mapping Each aggregate`` into a Receipt Item.
\$\endgroup\$
  • \$\begingroup\$ Hey thank you very much for the information. And yeah I do agree that the code failed in many ways. Hopefully, I don't make these kind of mistakes in the future. thank you. Appreciated :) \$\endgroup\$ – Juggernaut May 21 '15 at 14:06
  • \$\begingroup\$ @Juggernaut You are welcome. I've added some additional, but perhaps rather boring additional information. \$\endgroup\$ – ben rudgers May 21 '15 at 18:07
7
\$\begingroup\$

First, did you ask what "an additional 5% tax" for imported goods means? Let's say £100 cost pre tax, 10% tax = £110, is the 5% taken from the £100 or from the £110? If you didn't ask that, that would be a major minus.

Second, you have three methods to calculate taxes in three out of four ways (10% tax + import tax, 10% tax + no import tax, just import tax) and assume that in the last case the tax is zero and doesn't need to be calculated. This is just not scaleable. If you have a zero, low, high and luxury tax rate, plus import tax from one set and from another set of countries, suddenly you have 12 different methods.

I'd have a tax calculation object that is initialised to the relevant information (basic tax rate yes or no, import tax) which can then return the tax to pay for any price. The interface is simple, just what the tax calculation object needs, the actual calculation is as complicated as it is, but hidden from the caller.

Note that the whole system might fail in other situations: For example if you import goods into the UK, you cannot just calculate the import tax for each item, because the tax to be paid depends on how you combine the items.

\$\endgroup\$
  • 1
    \$\begingroup\$ For the first one: They gave me output and input .. So, I just calculated 10% and 5% separately one after other with the calculator and also just 15% on a whole rather than calculating individually. I got the correct output for the given input for the later one(15% on a whole). So, I thought there is no need of asking them how to calculate the tax. \$\endgroup\$ – Juggernaut May 21 '15 at 17:12
  • \$\begingroup\$ The method of calculation was provided in the example. However, I think the idea of encapsulating the tax calculation is a good. This is where much the change to program logic will occur and isolating it will facilitate maintenance over the long term. \$\endgroup\$ – ben rudgers May 22 '15 at 16:02
  • \$\begingroup\$ I believe this is really the key point. You must be able to think through a problem correctly before you can possibly program it. Three methods for two types of taxes is not scalable, it's true, but more importantly is the fact that it's something that the author of the code should spot and improve—not the requester of the software. \$\endgroup\$ – Wildcard Apr 19 '16 at 7:44
6
\$\begingroup\$

When I read the code, I thought to myself,

  • Why use enum when a bool is sufficient?
  • Why use 3 interfaces as well as a class?
  • Why three types of tax? Having 3 types of tax wasn't in the spec.
  • Why is the Generate method making decisions based on the type of product?
  • What data is in the Receipt when you finish? Is the Receipt creating products?

I like code that's "the simplest thing that could possibly work", so I'd write something like,

class Taxes
{
    readonly bool isSalesTax;
    readonly bool isImported;

    // alternatively instead of isSalesTax parameter,
    // could construct with a ProductType instance,
    // where ProductType has boolean properties like IsFood, IsMedicine, etc.
    internal Taxes(bool isSalesTax, bool isImported)
    {
        this.isSalesTax = isSalesTax;
        this.isImported = isImported;
    }

    internal decimal SalesTax(decimal price)
    {
        decimal rc = (isSalesTax) ? (price * 0.1M) : 0;
        // the spec says to round up to the nearest 0.05
        rc = Math.Ceiling(rc * 20) / 20;
        return rc;
    } 

    internal decimal ImportTax(decimal price)
    {
        return (isImported) ? (price * 0.05M) : 0;
    } 

    internal decimal TotalTax(decimal price)
    {
        return SalesTax(price) + ImportTax(price);
    } 
}

class Product
{
    // data from constructor parameters
    internal string Name { get; private set; }
    internal decimal Price { get; private set; }
    readonly Taxes taxes;
    // calculated properties
    internal decimal SalesTax { get { return Taxes.SalesTax(Price); } }
    internal decimal Total { get { return Price + Taxes.TotalTax(Price); } }

    internal Product(string name, decimal price, Taxes taxes)
    {
        this.Name = name;
        this.Price = price;
        this.taxes = taxes;
    }
}

class Receipt
{
    readonly List<Product> products;
    internal Receipt(IEnumerable<Product> products)
    {
        // use .ToList() to make a copy of the passed-in products
        this.products = products.ToList();
    }
    void Show()
    {
        decimal salesTaxes = 0;
        decimal total = 0;

        foreach (Product product in products)
        {
            salesTaxes += product.SalesTax;
            total += product.Total;

            Console.WriteLine(...this product...)
        }
        Console.WriteLine(...the two totals...)
    }
}

Note:

  • Tax logic is self-contained (in Taxes class)
  • Product contains a Taxes instance for tax calculations
  • To obey the Law of Demeter, Product exposes SalesTax and Total properties instead of exposing its Taxes instance.
  • Receipt doesn't create the list of products: whoever creates the Receipt passes the products as a constructor parameter
  • Receipt knows nothing about how taxes are calculated.
  • You define 9 types (ProdType, TaxStatus, Product, etc.) in the OP; I define 3.

Begging your pardon but just to run it home, criticism of your OP is:

  • Needlessly complex (e.g. enum instead of bool, and those abstract interfaces add nothing and could as well be deleted)
  • System cannot (as-is) generate 3 receipts for 3 different lists of product; editing and recompiling lets you generate one receipt for any one of 3 lists
  • The Product definition takes too much vertical space
  • Altering the Tax class isn't enough to change the taxation algorithm because there is, also, more taxation logic in the Generate method.
  • The value of the TotalTaxes member data will vary depending on how many times the Generate method is called. There's only method which is guaranteed to be called once and only once, i.e. the constructor. So either your Generate method should be class constructor, or TotalTaxes should be a local variable of the Generate method (not a data member of the class).
  • The spec says there's a rounding rule for sales tax but not for import tax. You're rounding the import tax as well.
  • a is a nonsense name for a variable.
  • There are three different lines where you invoke new Tax(p.Price) (and upcast to an interface) which is so needlessly complex that I wonder whether/why you think it's necessary.
  • You initialize but don't use the tax data member of the Tax class.
  • Because you store TaxStatus in Product you have no data/state to store in the Tax class; it might as well be a static class with static methods which take price as method parameters instead of as a constructor parameter
  • You have a property named TaxStatus TaxStatus (name equals type) and another named ProdType Typeofproduct (name not equals type). I prefer it when a programmer uses regular/consistent naming conventions.

Any suggestion to "show OOP in the code i write" would be appreciated.

Well I could modify the above to have an IProduct interface and an ITaxes interface, to that Receipt could work with an enumeration of IProduct instances (instead of depending on a specific Product type) and so that Product could work with an ITaxes instance (instead of working with a specific Taxes type).

That said there's more to OOP than inheritance. The most basic principle is to hide implementation inside a class, so if you need to change a spec or change the implementation of a spec then you only need to change one class.

For example I've been somewhat successful at encapsulating most of their spec inside my Taxes class.

\$\endgroup\$
2
\$\begingroup\$

First of all this is incorrect: 1 imported box of chocolates: 11.85 <- Correct 11.25 * 1.05 = 11.81 (rounded)

There is a base abstract class Item and 4 concrete classes Book, Medical, Food and Other, IsImported is an Item bool Property

There is a base abstract class SalesTax and 2 concrete classes BasicTax, ImportTax

There is an Exemptions List for Items 10% tax free

There are 3 ways (in this example) in order to use Dependy Injection 1)using poor man DI, 2) using Ninject and 3) using SimpleInjector

Class Receipt works with Constructor Dependency Injection with the list of Taxes and the Repository (list of items). In class Receipt in order to calculate taxes there is a loop over "product list" and inside "product list" (grouped by name so I have also the quantity of same item) a loop over "taxesList" and method "CalculateTax"is invoked against every Item/Quantity

Can do unit test thanks to Dependency Injection

Net Features Used: Astract Classes/Inheritance/Dipendency Injection/Linq/IOC (Ninject - Simple Injector)

using System;
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using System.Runtime;
using System.Text;
using SimpleInjector;

namespace CreateReceiptConsole
{
    class Program
    {
        private static void Main(string[] args)
        {
            Console.WindowWidth = 120;
            Console.WindowHeight = 20;

            try
            {
                //1)Using Poor Man DI (No IOC Container)

                ItemsRepository productList = new SqlRepository();
                IList<SalesTax> taxesList = new List<SalesTax>
                {
                    new BasicTax(),
                    new ImportTax()
                };

                var receipt = new Receipt(productList, taxesList);
                receipt.CreateReceipt();
                Console.Read();

                //2) Using Ninject

                //using (var kernel = new StandardKernel())
                //{
                //kernel.Bind(r => r
                //.FromThisAssembly()
                //.SelectAllClasses()
                //.InheritedFrom<SalesTax>()
                //.BindAllBaseClasses()
                //.Configure(x => x.InSingletonScope()));
                //kernel.Bind<ItemsRepository>().To<SqlRepository>();
                //var receipt = kernel.Get<Receipt>();
                //receipt.CreateReceipt();
                //Console.Read();
                //}

                //3) Using Simple Injector IOC Container/Framework

                //using (var container = new Container())
                //{
                //    container.Register<ItemsRepository, SqlRepository>(Lifestyle.Singleton);
                //    container.RegisterCollection(typeof(SalesTax), typeof(SalesTax).Assembly);
                //    container.Verify(VerificationOption.VerifyAndDiagnose);
                //    var receipt = container.GetInstance<Receipt>();
                //    receipt.CreateReceipt();
                //    Console.Read();
                //}
            }
            catch (Exception ex)
            {
                Console.WriteLine(ex.ToString());
            }
        }
    }

    public class Receipt
    {
        private readonly IList<SalesTax> taxesList;
        private readonly ItemsRepository repository;

        public Receipt(ItemsRepository repository, IList<SalesTax> taxesList)
        {
            if (repository == null)
            {
                throw new ArgumentNullException(@"repository");
            }
            if (taxesList == null)
            {
                throw new ArgumentNullException(@"taxesList");
            }

            this.repository = repository;
            this.taxesList = taxesList;
        }

        public void CreateReceipt()
        {
            var itemsGroups = repository.Getproducts().OrderBy(item => item.GetType().Name).GroupBy(item => item.Name).ToList();

            decimal total = 0;
            decimal totalTaxes = 0;
            int totalItems = 0;
            StringBuilder sbLine = new StringBuilder();
            StringBuilder sbItemName = new StringBuilder();
            StringBuilder sbItemCategory = new StringBuilder();

            for (int i = 0; i < itemsGroups.Count; i++)
            {
                total += itemsGroups[i].Sum(p => p.Price);
                totalItems += itemsGroups[i].Count();
                decimal itemTaxes = taxesList.Sum(t => t.CalculateTax(itemsGroups[i].First(), itemsGroups[i].Count()));
                totalTaxes += itemTaxes;
                sbItemName.Append(itemsGroups[i].Key.PadRight(10, ' '));
                sbItemCategory.Append(itemsGroups[i].First().GetType().Name.PadRight(8, ' '));
                var itemCount = itemsGroups[i].Count();
                var itemPriceSum = itemsGroups[i].Sum(p => p.Price);

                sbLine.Append(@" Name: " + sbItemName.ToString().PadRight(26, ' ') + @" - Category: " + sbItemCategory + @" - Items: " + itemCount.ToString().PadRight(2, ' ') + @"- Price: " + itemPriceSum.ToString(CultureInfo.InvariantCulture).PadLeft(5, ' ') + " - Taxes: " + itemTaxes.ToString("F2") + " - Total: " + (itemPriceSum + itemTaxes).ToString("F2"));

                Console.BackgroundColor = i % 2 == 0 ? ConsoleColor.Black : ConsoleColor.White;
                PrintReceipt.PrintToScreen(sbLine.ToString(), i%2 == 0 ? ConsoleColor.White : ConsoleColor.Blue);
                sbLine.Clear();
                sbItemName.Clear();
                sbItemCategory.Clear();
            }

            sbLine.AppendLine("");
            sbLine.AppendLine("");
            sbLine.AppendLine(" Total Items: " + totalItems.ToString("F2"));
            sbLine.AppendLine(" Total Taxes: " + totalTaxes.ToString("F2"));
            sbLine.AppendLine(" Total Price: " + (total + totalTaxes).ToString("F2"));
            Console.BackgroundColor = ConsoleColor.Black;
            PrintReceipt.PrintToScreen(sbLine.ToString(), ConsoleColor.Red);        
        }
    }

    public abstract class Item
    {
        public decimal Price { get; }
        public string Name { get; }
        public bool IsImport { get; }

        protected Item(string name, decimal price, bool isImport)
        {
            Name = name;
            Price = price;
            IsImport = isImport;
        }

        public override string ToString() => Name;
    }

    public class Book : Item
    {
        public Book(string name, decimal price, bool isImport) : base(name, price, isImport) { }
    }

    public class Medical : Item
    {
        public Medical(string name, decimal price, bool isImport) : base(name, price, isImport) { }
    }

    public class Food : Item
    {
        public Food(string name, decimal price, bool isImport) : base(name, price, isImport) { }
    }

    public class Other : Item
    {
        public Other(string name, decimal price, bool isImport) : base(name, price, isImport) { }
    }

    public abstract class SalesTax
    {
        protected string Description { get; }
        protected decimal Rate { get; }
        protected IList<Type> ExemptionsList { get; }

        protected SalesTax()
        {
            ExemptionsList = Exemptions.GetExemptionsList();
            Rate = TaxRateAndDescription.GetTaxRate(this);
            Description = TaxRateAndDescription.GetTaxDescription(this);
        }

        protected bool IsImport(Item item)
        {
            return item.IsImport;
        }

        public abstract decimal CalculateTax(Item item, int quantity);

        public override string ToString() => $"Description: {Description} -  Rate {Rate}";
    }

    public class BasicTax : SalesTax
    {
        public override decimal CalculateTax(Item item, int quantity)
        {
            return ExemptionsList.Contains(item.GetType()) ?
                0m : decimal.Round(item.Price * quantity * Rate, 2, MidpointRounding.AwayFromZero);
        }
    }

    public class ImportTax : SalesTax
    {
        public override decimal CalculateTax(Item item, int quantity)
        {
            return IsImport(item) ?
                decimal.Round(item.Price * quantity * Rate, 2, MidpointRounding.AwayFromZero) : 0m;
        }
    }

    public abstract class ItemsRepository
    {
        public abstract IList<Item> Getproducts();
    }

    public class SqlRepository : ItemsRepository
    {
        public override IList<Item> Getproducts()
        {
            return new List<Item>
            {
                new Other("imported bottle of perfume", 27.99m, true),
                new Other("bottle of perfume", 18.99m, false),
                new Medical("packet of headache pills", 9.75m, false),
                new Food("box of imported chocolates", 11.25m, true)
            };
        }
    }

    public class TaxRateAndDescription
    {
        public static decimal GetTaxRate(SalesTax tax)
        {
            switch (tax.GetType().Name)
            {
                case "BasicTax":
                    return 0.1m;
                case "ImportTax":
                    return 0.05m;
                default:
                    return 0.1m;
            }
        }

        public static string GetTaxDescription(SalesTax tax)
        {
            switch (tax.GetType().Name)
            {
                case "BasicTax":
                    return "BasicTax";
                case "ImportTax":
                    return "ImportTax";
                default:
                    return "BasicTax";
            }
        }
    }

    public class Exemptions
    {
        public static List<Type> GetExemptionsList()
        {     
            return new List<Type>
            {
                typeof(Book),
                typeof(Food),
                typeof(Medical)
            };
        }
    }

    public class PrintReceipt
    {
        public static void PrintToScreen(string debug, ConsoleColor color)
        {
            Console.ForegroundColor = color;
            Console.WriteLine(debug);
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ This answer is not much of a review of the posted code. You pointed out an error in the posted code, and provided an alternative implementation. An alternative implementation can be very useful to demonstrate the main points explained in the answer. But it's rarely useful as the main feature. You need to explain more about the posted code. Please read the how-to-answer page in the help center. \$\endgroup\$ – janos Apr 19 '16 at 8:01
  • \$\begingroup\$ Thanks for your answer janos, you're totally right. It's been easier to me re-write the code as reviewing it would have take too long (to me). Thanks \$\endgroup\$ – AngelBlueSky Apr 19 '16 at 8:41
  • \$\begingroup\$ Your answer is much better than it originally was. When it was deleted, it was a code dump, with no explanation. Adding the explanation improves it. I see two code review points: 1. you point out an error, and 2. you recommended reorganizing the code to make it unit testable. You're going in a good direction here, you have my vote ;-) \$\endgroup\$ – janos Apr 19 '16 at 8:51
  • \$\begingroup\$ Thanks a lot i appreciate it, sometimes I haven't got much time for writing all but I understand It could help better if I comment more. \$\endgroup\$ – AngelBlueSky Apr 19 '16 at 8:59

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.