Lista C# - Ćwiczenie

Hej,

Robię ćwiczenie na listach i mam problem bo po stworzeniu samochodu i dodaniu do niego silnika i nadwozia nie wiem jak sie "dobrać" do tych danych tak, żeby sobie wypisać wszystko co sie znajduje w stworzonym samochodzie?

To są moje początki w C# i w sumie to nawet nie jestem pewien czy dobrze sobie tworze te klasy :)

Będe wdzięczny za wszystkie sugestie :)

namespace Fabryka
{
    abstract class Produkt
    {
        public int ID;

        public virtual void Dodaj()
        {
            Random n = new Random();
            ID = n.Next(Int32.MaxValue);
        }
        public abstract void Wypisz();
        public abstract void Usun();
    }
    class Samochod:Produkt
    {
        public string Marka;
        public string Model;
        public decimal Cena;
        public double RokProdukcji;

        public override void Dodaj()
        {
            base.Dodaj();
            Console.Write("Marka = ");
            Marka = Console.ReadLine();
            Console.Write("Model = ");
            Model = Console.ReadLine();
            Console.Write("Cena = ");
            Cena = Convert.ToDecimal(Console.ReadLine());
            Console.Write("Rok produkcji = ");
            RokProdukcji = Convert.ToInt32(Console.ReadLine());
            BazaSamochodow.S.Add(this);
        }

        public override void Wypisz()
        {
            Console.WriteLine(Marka + Model);
        }

        public override void Usun()
        {
            BazaSamochodow.S.Remove(this);
        }

        class Silnik : Samochod
        {
            public string RodzajSilnika;
            public override void Dodaj()
            {
                Console.WriteLine("\nDodawanie silnika: ");
                Console.Write("Rodzaj silnika = ");
                RodzajSilnika = Console.ReadLine();
                BazaSamochodow.S.Add(this);
            }

            public override void Wypisz()
            {
                Console.WriteLine(RodzajSilnika + Model + Marka );
            }

            public override void Usun()
            {
            }
        }
        class Nadwozie : Samochod

        {
            public string RodzajNadwozia;
            public override void Dodaj()
            {

                Console.Write("Rodzaj nadwozia = ");
                RodzajNadwozia = Console.ReadLine();
                BazaSamochodow.S.Add(this);

            }

            public override void Wypisz()
            {
               // Console.WriteLine(RodzajNadwozia);
            }

            public override void Usun()
            {

            }

        }
        class BazaSamochodow
        {
            public static List<Produkt> S = new List<Produkt>();
            public static void WypiszWszystkie()
            {
                foreach (var x in S)
                {
                    Console.WriteLine(); // myślałem, że zrobie (x.ID + x.Marka + x.RodzajNadwozia) ale to by było za piękne - widzi tylko x.ID ;)
                }
            }

Przypisujesz do ID jakąś losową liczbę, ale nie sprawdzasz czy już nie masz produktu o takim ID.

Nadwozie czy silnik dziedziczy po samochodzie? Nie wydaje mi się.

Klasa produkt nie masz przecież takich pól jak marka czy rodzaj nadwozia, wiec skąd chcesz je odczytać?

Nie sądzę, żeby taka organizacja klas była dobra. Jako abstrakcję masz klasę Samochód, która ma metody Dodaj i Usuń. Okej, ale dodaj gdzie, usuń skąd? W klasach potomnych ich implementacje są nadpisane i dodają się same gdzieś do jakiejś listy statycznej. To bardzo zły pomysł bo coś się wydarzy jak zrobię coś takiego: "BazaSamochodow.S = null;" Za każdym razem będzie wyjątek.
Druga sprawa to samo dziedziczenie. Klasy Nadwozie i Silnik dziedziczą po klasie Samochód. Ani nadwozie ani silnik nie są samochodami. Powinieneś to zrobić tak: mieć klasę abstrakcyjną (albo interfejs) Silnik po której dziedziczą konkretne implementacje silników, tak samo dla nadwozia. Potem w klasie Samochód mieć pola typu Silnik i typu Nadwozie gdzie masz informacje jaki konkrenty silnik i jakie konkretne nadwozie ma twój samochód.
A listę w klasie BazaSamochodow robisz prywatną i udostępniasz publicznie tylko metody do dodawania, usuwania i odczytu. Wtedy nawet jak lista będzie statyczna to nikt nigdzie jej nie zepsuje.

Dzięki za odpowiedzi :) Faktycznie bardzo zepsułem ta organizacje klas. Napisałem sobie to na nowo i działa to co chciałem uzyskać :) Teraz moge spokojnie poćwiczyć linq.
Musze jeszcze przeczytać sporo teorii żeby to sobie wszystko ogarnąć. Jak oceniacie teraz ten kod? Chciałem też zapytać czy ma to jakieś znaczenie gdzie sobie wrzuce "BazaSamochodow.S.Add(this);" bo zastosowanie tej funkcji w innym voidzie nie robi różnicy, dane i tak sie dodają do listy.
Jak podszkole teorie to pobawie sie też w zabezpieczenie tych danych bo zdaje sobie sprawe, że to wszystko takie "dostępne" póki co :)

namespace FabrykaSamochodow
{
    class Samochod
    {
        public int ID;
        public string Marka;
        public string Model;
        public decimal Cena;
        public int RokProdukcji;
        public string RodzajNadwozia;
        public string RodzajSilnika;

        public void DodajSamochod()
        {
            Console.WriteLine("Dodawanie samochodu: \n");
            Random n = new Random();
            ID = n.Next(Int32.MaxValue);
            Console.Write("Marka = ");
            Marka = Console.ReadLine();
            Console.Write("Model = ");
            Model = Console.ReadLine();
            Console.Write("Cena = ");
            Cena = Convert.ToDecimal(Console.ReadLine());
            Console.Write("Rok produkcji = ");
            RokProdukcji = Convert.ToInt32(Console.ReadLine());
        }

        public void DodajSilnik()
        {
            Console.WriteLine("\nDodawanie silnika: \n");
            Console.Write("Rodzaj silnika = ");
            RodzajSilnika = Console.ReadLine();
        }

        public void DodajNadwozie()
        {
            Console.WriteLine("\nDodawanie nadwozia: \n\n");
            Console.Write("Rodzaj nadwozia = ");
            RodzajNadwozia = Console.ReadLine();
            BazaSamochodow.S.Add(this);
        }

    }

    class BazaSamochodow : Samochod
    {
        public static List<Samochod> S = new List<Samochod>();
        public static void WypiszWszystkie()
        {
            foreach (var x in S)
            {
                Console.WriteLine("ID: " + x.ID + "Marka: " + x.Marka + "Model: " + x.Model + "Rok produkcji: " + x.RokProdukcji + "Cena: " + x.Cena + "Rodzaj silnika: " + x.RodzajSilnika + "Rodzaj nadwozia: " + x.RodzajNadwozia);
            }
        }

}

Nadal nie jest dobrze. Po pierwsze, po co BazaSamochodow dziedziczy po klasie Samochod? Czy BazaSamochodow ma swój rok produkcji, rodzaj silnika czy rodzaj nadwozia?
Po drugie, w klasie Samochod masz metodę DodajSamochod, w której pobierasz z konsoli pewne podstawowe informacje, w kolejnej metodzie pobierasz kolejną porcję danych i na koniec masz trzecią metodę, która również pobiera jakieś informacje, ale na końcu dodaje samochód do jakieś wspólnej kolekcji. A metody te będą wywołane w innej kolejności albo ta ostatnie nie zostanie wywołana w ogóle?
Po trzecie, klasa Samochod w tej ostatniej metodzie dodaje instancję samej siebie do określonej kolekcji. Samochod nie powinien się martwić o dodawanie samego do jakiejkolwiek kolekcji.
Po czwarte, twoja klasa Samochod sama pobiera dane o sobie z konsoli. A co jak będziesz chciał, żeby ta klasa współpracowała z jakimś innym interjesem użytkownika?

Generalnie, moja rada jest taka: poczytaj o podstawach programowania obiektowego, chociażby to co masz tutaj: Programowanie obiektowe