8
\$\begingroup\$

One year ago I published an F# solution of the same task and there is an old C# solution. But I think it's a simple task and require a simple solution.

What do you think?

using System;
using System.Collections.Generic;

namespace SalesTaxes
{
    class Program
    {
        static void Main(string[] args)
        {
            List<ShoppingCartItem> itemList = getItemsList();

            decimal salestaxes = 0.00m;
            decimal totalprice = 0.00m;

            foreach (ShoppingCartItem item in itemList)
            {
                salestaxes += item.Taxes * item.Quantity;
                totalprice += item.Item.Price * item.Quantity;
                Console.WriteLine(string.Format("{0} {1} : {2}", item.Quantity, item.Item.Name, (item.Item.Price + item.Taxes) * item.Quantity));
            }
            totalprice += salestaxes;
            Console.WriteLine("Sales Taxes : " + salestaxes);
            Console.WriteLine("Total : " + totalprice);
            Console.ReadLine();
        }

        private static List<ShoppingCartItem> getItemsList()
        {
            List<ShoppingCartItem> lstItems = new List<ShoppingCartItem>();
            //input 1

            lstItems.Add(new ShoppingCartItem { Item = new Product { Name = "Book", Price = 12.49m, Type = Product.ProductType.book, IsImport = false }, Quantity = 1 });
            lstItems.Add(new ShoppingCartItem { Item = new Product { Name = "music CD", Price = 14.99m, Type = Product.ProductType.other, IsImport = false }, Quantity = 1 });
            lstItems.Add(new ShoppingCartItem { Item = new Product { Name = "chocolate bar", Price = 0.85m, Type = Product.ProductType.food, IsImport = false }, Quantity = 1 });

            return lstItems;
        }
    }

    public class Product
    {
        public enum ProductType
        {
            food = 1,
            book = 2,
            medical = 3,
            other = 4
        };

        public string Name { get; set; }        
        public decimal Price { get; set; }
        public ProductType Type { get; set; }
        public bool IsImport { get; set; }
        public bool IsExempt
        {
            get
            {
                return (int)Type < 4;
            }
        }
    }    

    public class ShoppingCartItem
    {
        const decimal TaxRate = 0.1m;
        const decimal ImpTaxRate = 0.05m;
        public Product Item { get; set; }
        public int Quantity { get; set; }
        public decimal Taxes
        {
            get
            {
                return decimal.Ceiling(Item.Price * ((Item.IsExempt ? 0 : TaxRate) + (Item.IsImport ? ImpTaxRate : 0)) * 20) / 20;
            }
        }
    }
}
\$\endgroup\$
12
\$\begingroup\$

A couple of small suggestions.

Var: You spend a lot of time redeclaring variable types when they are already well established by your code. For example:

decimal totalprice = 0.00m;
foreach (ShoppingCartItem item in itemList){...}
List<ShoppingCartItem> lstItems = new List<ShoppingCartItem>();

I know there can be some debate about whether var is preferred or not, but I can say I work in a shop that currently uses it heavily and it makes refactoring a dream. Just changing the Foreach to:

foreach(var item in itemList)

would make refactoring much easier later. This way it will figure out the type for you at compile time, meaning itemList can be an ienumerable of anything as would still be valid in that case.

Console.WriteLine: There is already a string format overload for Console.Writeline. The following lines are equivalent:

Console.WriteLine(string.Format("{0} ", x));
Console.WriteLine("{0}",x);

Collection Construction: You can use simpler syntax to construct your collection in getItemsList() making your entire signature for the function something closer to this:

    private static IEnumerable<ShoppingCartItem> getItemsList()
    {
        return new List<ShoppingCartItem>
        {
            new ShoppingCartItem
            {
                Item = new Product {Name = "Book", Price = 12.49m, Type = Product.ProductType.book, IsImport = false},
                Quantity = 1
            },
            new ShoppingCartItem
            {
                Item = new Product{Name = "music CD",Price = 14.99m,Type = Product.ProductType.other,IsImport = false},
                Quantity = 1
            },
            new ShoppingCartItem
            {
                Item = new Product{Name = "chocolate bar",Price = 0.85m,Type = Product.ProductType.food,IsImport = false},
                Quantity = 1
            }
        };
    }

Return Type List: For what you use the return value for, in getItemsList, it doesn't need to be returned as a List. It could be an IList or IEnumerable without any ill effects. It's not a huge deal in your case as it's private, but around where I work we generally try to return collections under there interfaces. I know resharper will flag this as well.

Naming: The enumeration ProductTypes values and the getItemsList method do not follow C# standards for naming. It really should be GetItemsList and Food/Book/Medical/Other.

IsExempt Why is it:

return (int)Type < 4;

instead of:

return Type != ProductType.Other;

They mean the same thing, but one is a bit more obvious as to it's intentions.

Taxes Getter: This is more of a soft suggestion, but this getter is a bit convoluted to read. I had to dig through and add some spacing to figure out what it was doing and even then I misplaced a paren the first time and got the wrong solution. I understand what you are doing with this type of code but it's honestly something that will be more of a headache than a help later as it is not immediately obvious what it is doing unless you dig in. Maybe simplifying out the ternary statements into getters for those values, or methods if you prefer, would be a better solution as it would vastly simplify the amount of parenthesis and make the statement a bit easier on the eyes. Example:

public decimal Taxes
    {
        get
        {
            return decimal.Ceiling( Item.Price *( CalculateTaxRate() + CalculateImportRate()) * 20) / 20;
        }
    }

    private decimal CalculateTaxRate()
    {
        return Item.IsExempt
            ? 0
            : TaxRate;
    }

    private decimal CalculateImportRate()
    {
        return Item.IsImport
            ? ImpTaxRate
            : 0;
    }

This seems much easier to read at a glance.

Writelines Also worth noting that you swap between the string.format method of writing variables out to the string concatenation method in a couple of places:

Console.WriteLine(string.Format("{0} {1} : {2}", item.Quantity, item.Item.Name, (item.Item.Price + item.Taxes) * item.Quantity));
Console.WriteLine("Sales Taxes : " + salestaxes);

Best to stick with a style when you start. I prefer the string format approach, using the appropriate Console.WriteLine overload, as it hedges away from doing string + string. In this case it's really not more efficient, but it's a bad to get into the habit of string + string, as in non-trivial usages of concatenation it's inefficient. When I find yourself using string + string it's usually a tip off that there is a better way to be doing it (StringBuilder, String.Format, etc). Once again, no actual gain from this in your instance, just a good habit to build upon.

Overall: The solution itself looks pretty good, though I'll admit I didn't run it through too much in the way of testing. I would just suggest those small style changes. Hope that helps.

\$\endgroup\$
  • 2
    \$\begingroup\$ 1. I really like your point of var embiggens refactoring. 2. I believe the enum test can be written return Type < ProductType.other; \$\endgroup\$ – radarbob Nov 12 '15 at 4:05
  • \$\begingroup\$ You can, I guess I would make that call based on how to intended to extend that enumeration. If food book and medical are the only ones which will ever need exemption then I would suggest your route as it would lead to all other non-exempt entries to the enumeration later to be correct. If all other entries would be exempt as well, my solution would probably be best. If unsure, I don't think it matters, though I think Id still default to an equality rather than a less than just because it fits my own mental model a bit better. That's just a taste issue though. \$\endgroup\$ – BlindGarret Nov 12 '15 at 14:52
  • 1
    \$\begingroup\$ I was just looking that the magic number - that statement is broken as soon as new enum members are added. Coding dependent on enum actual values is a time bomb. I've done it - but document the hell out of it. Hide it. Unit test it, etc. etc. With my change the bomb is still ticking but the code explains itself. Either way I agree, @BlindGarret, that your suggestion is better here. \$\endgroup\$ – radarbob Nov 12 '15 at 16:11

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.