3
\$\begingroup\$

This is not homework. I am trying to learn OOD principles and design patterns myself.

Suppose I have a problem like this:

A book-shop buys and sells two types of books:

  1. Non-technical {Title, Author, Price}
  2. Technical {Title, Author, Price, CD}

Also, customer gets a CD when he buys a Technical book. A CD object is defined as, CD {Title, Price}.

A Non-technical book's price will be only the price of the book. A Technical book's price will be the sum of the price of the book and the CD.

Create a C# program to show the following info:

  • Total number of book Bought & Price: XXX & XXX.XX
  • Total number of book Sold & Price: XXX & XXX.XX
  • Total Technical Book Sold & Price: XXX & XXX.XX
  • Total Non-technical Book sold & Price: XXX & XXX.XX

I have designed the program like this:

abstract class Publication
{
    public virtual string Title { get; set; }
    public virtual double Price { get; set; }
}

class CD : Publication
{
}

abstract class Book : Publication
{
    public virtual string Author { get; set; }
}

class TechnicalBook : Book
{
    public CD Cd { get; set; }
    public override double Price
    {
        get
        {
            return (base.Price + Cd.Price);
        }
    }
}

class NonTechnicalbook : Book
{
}

abstract class Shop
{
    private IDictionary<string, Book> boughtDictionary;
    private IDictionary<string, Book> soldDictionary;

    public Shop()
    {
        boughtDictionary = new Dictionary<string, Book>();
        soldDictionary = new Dictionary<string, Book>();
    }

    public virtual void Buy(Book item)
    {
        boughtDictionary.Add(item.Title, item);
    }

    public virtual void Sell(string title)
    {
        Book book = boughtDictionary[title];
        boughtDictionary.Remove(book.Title);
        soldDictionary.Add(book.Title, book);
    }

    public virtual int GetBoughtBookCount()
    {
        return boughtDictionary.Count;
    }

    public virtual double GetBoughtBookPrice()
    {
        double price = 0.0;

        foreach (string title in boughtDictionary.Keys)
        {
            price = price + boughtDictionary[title].Price;
        }
    }

    public virtual int GetSoldBookCount()
    {
        return boughtDictionary.Count;
    }

    public virtual double GetSoldBookPrice()
    {
        double price = 0.0;

        foreach (string title in soldDictionary.Keys)
        {
            price = price + soldDictionary[title].Price;
        }
    }

    public virtual double GetTotalBookCount()
    {
        return this.GetBoughtBookCount() + this.GetSoldBookCount();
    }

    public virtual double GetTotalBookPrice()
    {
        return this.GetBoughtBookPrice() + this.GetSoldBookPrice();
    }

    public virtual void Show()
    {
        Console.WriteLine("Total number of books Bought & Price: ", this.GetTotalBookCount() + " & " + this.GetTotalBookPrice());
        Console.WriteLine("Total number of books Sold & Price: ", this.GetSoldBookCount() + " & " + this.GetSoldBookPrice());
    }
}

Does this design conform to the Open-Closed principle? Now I am unable to understand how to separate Technical and Non-technical books at this point.

\$\endgroup\$
4
\$\begingroup\$

Overriding the Price property in TechnicalBook modifies the behavior of Publication. Which means you've broken the OCP. Besides, what do you expect TechnicalBook.Price to be if you have just set it to 10.0? (Does it even compile when you only override get?)

What you could do is employ the template method pattern:

abstract class Publication
{
    // ...
    public decimal IndividualPrice { get; set; }
    public decimal Price {
        get { return CalculatePrice(); }
    }

    protected decimal CalculatePrice() {
        decimal price = IndividualPrice;
        price += GetAdditionalPrices();
        return price;
    }

    protected virtual decimal GetAdditionalPrices() {
        return 0;
    }
}

public class TechnicalBook : Publication
{
    // ...
    protected override decimal GetAdditionalPrices() {
        return Cd.Price;
    }
}

This makes Publication closed for modification (cannot change price / individual price), but open for extension (can add to price).

(Naming and structure in my example could probably be improved, but it serves the purpose)

The behavior is still possible to modify, though, so an even better solution would be to have some collection or set of rules you can add to, so you don't accidentaly override GetAdditionalPrices without calling base.GetAdditionalPrices().

\$\endgroup\$
  • \$\begingroup\$ What about my second question? How to show the prices of Technical and Non-technical books separately? \$\endgroup\$ – user3804 Feb 9 '12 at 14:54
  • \$\begingroup\$ For that you could for instance use LINQ. var technicalBookTotal = soldBooks.Values.OfType<TechnicalBook>.Sum(b => b.Price); \$\endgroup\$ – Lars-Erik Feb 9 '12 at 15:39
0
\$\begingroup\$

I will not address the Open-Closed principle, as I do not know it.

How to separate Technical from NonTechnical books

This second question belongs in StackOverflow.
Yet another reason to open a separate question for each real question you have...

But you can do something like:

if(book is TechnicalBook) {
    // ...
} else if(book is NonTechnicalBook) {
    // ...
} else {
    // This is why I would suggest Book class not being abstract...
    // and removing the NonTechnicalBook class, making the TechnicalBook extend Book.
    throw new ArgumentException();
}

Remarks on the code

  1. public CD Cd should be either Cd Cd or CD CD.

  2. I don't see the advantage of Having the definition for NonTechnicalbook. Couldn't you just use regular books, if they were not abstract?
    But perhaps this is part of the principle you mention.

  3. IDictionary<string, Book> boughtDictionary; What if I buy two books titled C# for Dummies? This should be a Dictionary<Book, int>.

  4. Having the word dictionary in a variable's name is Hungarian notation - which I would avoid.

  5. What if I try to sell an inexistent book? KABOOM!

Have you tried running your code? Case in point: - Your Console.WriteLine() is not working as you expect it to, you have a comma instead of a plus; - Some methods don't even return a value, and so do not compile; - The GetSoldBookCount is using the boughtDictionary; - Etc.

It sure smells like homework.

  1. Some of

This sure feels like homework.

Revised code suggestion

(Sorry about the brackets indentation. Too bothersome to change it back to your style.)

namespace BookSellers
{
    using System.Linq;
    abstract class Publication
    {
        public virtual string Title { get; set; }
        public virtual double Price { get; set; }
    }

    class CD : Publication
    {
    }

    abstract class Book : Publication
    {
        public virtual string Author { get; set; }
    }

    class TechnicalBook : Book
    {
        public CD CD { get; set; }
        public override double Price {
            get {
                return (base.Price + CD.Price);
            }
        }
    }

    class NonTechnicalbook : Book
    {
    }

    abstract class Shop
    {
        private IDictionary<Book, int> BooksInStock;
        private IDictionary<Book, int> BooksSold;

        public Shop() {
            BooksInStock = new Dictionary<Book, int>();
            BooksSold = new Dictionary<Book, int>();
        }

        public virtual void Buy(Book book) {
            int count;
            if (!BooksInStock.TryGetValue(book, out count)) {
                count = 0;
            }
            BooksInStock[book] = count + 1;
        }

        public virtual void Sell(Book book) {
            int count;
            if (!BooksInStock.TryGetValue(book, out count)
                || count < 1
                ) {
                throw new ArgumentException("Book is not in stock.", "book");
            }
            BooksInStock[book] = count - 1;

            if (!BooksSold.TryGetValue(book, out count)) {
                count = 0;
            }
            BooksSold[book] = count + 1;
        }

        public virtual int GetTotalBooksCount() {
            return GetStockCount() + GetSoldBooksCount();
        }
        public virtual int GetStockCount() {
            return BooksInStock.Values.Sum();
        }
        public virtual int GetSoldBooksCount() {
            return BooksSold.Values.Sum();
        }

        public virtual double GetTotalBooksPrice() {
            return GetStockBooksPrice() + GetSoldBooksPrice();
        }
        public virtual double GetStockBooksPrice() {
            return BooksInStock.Sum(pair => pair.Key.Price * pair.Value);
            /* Same as:
            double price = 0.0;
            foreach (var pair in BooksInStock) {
                Book book = pair.Key;
                int count = pair.Value;
                price += book.Price * count;
            }
            return price;*/
        }
        public virtual double GetSoldBooksPrice() {
            return BooksSold.Sum(pair => pair.Key.Price * pair.Value);
        }

        public virtual void Show() {
            Console.WriteLine(string.Format(
                "Total number of books Bought & Price: {0} & {1}",
                GetTotalBooksCount(),
                GetTotalBooksPrice()
                ));
            Console.WriteLine(string.Format(
                "Total number of books Sold & Price: {0} & {1}",
                GetSoldBooksCount(),
                GetSoldBooksPrice()
                ));
        }
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy