Adding new articles

Question

Following is a working program to add new articles. The only thing I want is to check if there is any way to write it in a better way, or if the architecture is good as it is.

Maybe I did not have to user a new class ListOfProducts I could easily create a list in the Articles class and add items directly in the BuyItem() method, but I just wanted to try inheritance and work on my skills.

Any other recommendation?

class Program
{
    static void Main(string[] args)
    {
        Guid id; 
        id = Guid.NewGuid();

        Articles article = new Articles(id, "This is a new article", 3, 20.2);

        article.BuyItem(article);

        Console.WriteLine(article);
        Console.ReadLine();
    }
}

public class Articles : ListOfProducts
{
    private Guid Id { get; set; }
    private string Description { get; set; }
    private int Quantity { get; set; }
    private double Price { get; set; }

    public Articles(Guid id, string description, int quantity, double price)
    {
        this.Id = id;
        this.Description = description;
        //should check if the quatity > 0
        this.Quantity = quantity;
        this.Price = price;
    }

    // add item
    public void BuyItem(Articles article)
    {
        AddNewArticle(this);
    }

    //Sell item
    public void SellItem(Articles article)
    {
        RemoveNewArticle(this);
    }

    public override string ToString()
    {
        return "Article: "+ Id+" is Added.";
    }
}

public class ListOfProducts
{
    private List<Articles> ListOfArticles = new List<Articles>();
    public void AddNewArticle(Articles art)
    {
        ListOfArticles.Add(art);        
    }

    public void RemoveNewArticle(Articles art)
    {
        ListOfArticles.Remove(art);
    }
}

Answer 1

The classes don't have a clear purpose. Every class should have a clear purpose, a single responsibility.

ListOfProducts

I originally wrote:

How is this class different from a List<Product>? Or a List<Article>? There's no visible difference. If there is no relevant difference, you don't need this class.

Then @radarbob pointed out in a comment:

I disagree with: How is this class different from a List<Product>? Or a List<Article>? ... you don't need this class. ListOfProducts.AddNewArticle() is in terms of the domain and that is a good thing. We can discuss class, methods names or specific implementation but those are side issues to the more fundamental design point of making classes in terms of the domain. I do this often. @janos, you say "each class ... have a clear purpose." What could be more clear than using "domain language" in the code that implements that domain?

He makes a good point. This class is fine. It uses composition to represent a collection of products, and it correctly doesn't inherit from List, that would expose more than intended (a full blown list interface). I would suggest renaming the class and methods to ProductList and AddArticle, RemoveArticle.

Articles

At first glance, the class seems to represent an article, with id, description, quantity, price properties. But that conflicts with the name, which implies some sort of collection. Indeed, it inherits from ListOfProducts which is some sort of collection. This dual responsibility is very strange, and clearly don't belong together. These responsibilities are very different, and should not be present in the same class.

It would make more sense to have an Article class that represents an article, and not inherit from ListOfProducts.

Also, looking at the fields of this class, there's nothing specific to an article. All fields are generic, that could be present in any product. As such, you could rename the class Product. If you want to practice inheritance, you could have an Article class that extends Product and adds some fields that are specific to an article.

The BuyItem and SellItem methods are essentially just aliases to List.Add and List.Remove methods. If these names are important, then you could put these methods on an ArticleList or ProductList class.

The purpose

The most important thing is to think of the purpose of each class, and make sure that they have a clear purpose and single responsibility. The resulting design will be natural and obvious.

Answer 2

am new to OOP bro, could you please show how would you design it:)?

public class Article {
    // these do not belong in the collection class.
    // they all have to do with a single Article

    private Guid Id { get; set; }
    private string Description { get; set; }
    private int Quantity { get; set; }
    private double Price { get; set; }

    public Article ( Guid id, string description, int quantity, double price ) {
         // Id is your key. Better make sure you have one
         if ( id == null )
             throw new ArgumentNullException ("Guid argument was null");

         ....
    }

    // if Guid is the identifier/key for an article then
    // you should override Equals

    public override bool Equals ( object other ) {
        Article theOther = other as Article;

        if ( theOther == null ) return false;

        return this.Id.Equals(other.Id);
    }

    // you should also override GetHashCode. But honestly
    // most of the time you can get away with not doing that.
    // but if you want more professional code (and fewer potential bugs), do it.

    public override void GetHashCode() {
        int hash = 17;
        // Suitable nullity checks etc, of course :)
        // repeat this line for every field used in Equals.
        hash = hash * 23 + Id.GetHashCode();
        return hash;
    }

    // use string.Format() instead of concatination ( ... + ... )
    // in general Format() is cleaner, easier to read, less typo error prone
    // and easier to modify w/out error.
    public override ToString () {
        // research about StringBuilder. It's preferred when stringing
        // together strings. 
        StringBuilder me = new StringBuilder();

        me.AppendFormat( "Article {0} is Added.", this.Id);
        me.AppendFormat( "\nDescription: {0}", this.Description );
        . . .

        return me.ToString();
    }
}

public class ArticleCollection {
    public List<Article> Articles { get; protected set; }

    public ArticleCollection() {
        Articles = new List<Article>();
    }

    public void AddArticle(Article newArticle) {
        if ( newArticle == null ) return;

        Articles.Add( new Article );
    }

    public void RemoveArticle(Article oldArticle) {
        if ( oldArticle == null ) return;
        . . .
    }

    //leverage Article.ToString()!
    public override string ToString(){
        StringBuilder me = new StringBuilder();

        foreach ( var article in Articles ) {
            me.AppendLine( article.ToString());
            me.AppendLine();  // a blank line between articles
        }

        return me.ToString();
    }


}

---

NOTE: The collection does not inherit:

public class ArticleCollection : List<Article> {}

Using composition instead of inheritance means that we've hidden all the public List<T> methods - so we can control how our collection is exposed to the client. And, we write methods in terms of our problem space.

The simpleAddArticle() above, for example, may seem redundant but that is not the point. Further we can add functionality, for example if we don't want duplicates, or there was some other criteria for adding or rejecting an Article we can do that. So our methods written in terms of our domain are customized for our domain as well.