Program sortujący - działa, ale chcę go poprawić

Witam,

Napisałem program sortujący podane przez użytkownika liczby. Działa, ale nie jest do końca taki, jaki powinien być. Zgodnie z zadaniem, elementami tablicy powinny być obiekty klasy Element. Dopiero zaczynam pracę z C# i książki, które czytam, nie pomagają mi tego ruszyć. Jakieś podpowiedzi/sugestie?

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace Projekt_1__konsola_
{
    sealed class Element
    {
        int val; 
        public Element(int e)
        {
            val = e;
        }
        public int v 
        {
            get
            {
                return val;
            }
        }

        static void PodajLiczbę(string komunikat, out int liczba)
        {
            while (true)
            {
                Console.Write(komunikat);
                string str = Console.ReadLine();
                try
                {
                    liczba = int.Parse(str);
                    break;
                }
                catch (FormatException)
                {
                    Console.ForegroundColor = ConsoleColor.Red;
                    Console.WriteLine("Wprowadzono liczbę w złym formacie");
                    Console.ForegroundColor = ConsoleColor.Black;
                }
                catch (OverflowException)
                {
                    Console.ForegroundColor = ConsoleColor.Red;
                    Console.WriteLine("Wartość jest za duża albo za mała, pamiętaj że możesz podać liczby z zakresu 1 do 4294967295");
                    Console.ForegroundColor = ConsoleColor.Black;
                }
                catch (ArgumentNullException)
                {
                    Console.ForegroundColor = ConsoleColor.Red;
                    Console.WriteLine("Napotkano koniec strumienia");
                    Console.ForegroundColor = ConsoleColor.Black;
                }
                Console.WriteLine("Spróbuj jeszcze raz");
            }
        }

        static void Sort(int[] tablica)
        {
            for (uint i = 1; i < tablica.Length; i++)
            {
                uint j = i;
                int buf = tablica[j];
                while ((j > 0) && (tablica[j - 1] > buf))
                {
                    tablica[j] = tablica[j - 1];
                    j--;
                }
                tablica[j] = buf;
            }
        }

        static void Main(string[] args)
        {
            int liczba, element, l, m;
            Console.Title = "Projekt"
            Console.BackgroundColor = ConsoleColor.White;
            Console.ForegroundColor = ConsoleColor.Black;
            Console.Clear();

            PodajLiczbę("Podaj liczbę elementów do posortowania: ", out liczba);
            int[] tablica = new int[liczba];
            for (l = 0; l < liczba; l++)
                {
                    PodajLiczbę("Podaj element [" + l + "]: ", out element);
                    for (m = 0; m <= tablica.Length; m++)
                    {
                        tablica[l] = element;
                    }   
                }

            Sort(tablica);

            Console.WriteLine("Posortowane elementy: ");
            for (l = 0; l < liczba; l++)
            {
                Console.WriteLine("Element [{0}] = {1}", l, tablica[l]);
            }
        }
    }
}

Zgodnie z zadaniem elementami tablicy powinny być obiekty klasy Element.

A u Ciebie cały program jest wywołaniem metody Main z klasy Element.
Zdaje się, że Twoja klasa Element powinna wyglądać mniej więcej tak (przy czym konstruktor jest w opcjonalny):

public class Element
    {
        public int Val { get; set; }

        public Element(int e)
        {
            Val = e;
        }
    }

A pozostała część:

class Program
    {
        private Element[] tablica;

        static void Main(string[] args)
        {
            //tutaj logika uzupełniania i tworzenia tablicy o określonym rozmiarze
            Sort(tablica);
        }

        static void Sort(Element[] tablica)
        {
            //tutaj logika sortowania
        }
    }

        static void PodajLiczbę(string komunikat, out int liczba)
        {
            while (true)
            {
                Console.Write(komunikat);
                string str = Console.ReadLine();
                try
                {
                    liczba = int.Parse(str);
                    break;
                }
                catch (FormatException)
                {
                    Console.ForegroundColor = ConsoleColor.Red;
                    Console.WriteLine("Wprowadzono liczbę w złym formacie");
                    Console.ForegroundColor = ConsoleColor.Black;
                }
                catch (OverflowException)
                {
                    Console.ForegroundColor = ConsoleColor.Red;
                    Console.WriteLine("Wartość jest za duża albo za mała, pamiętaj że możesz podać liczby z zakresu 1 do 4294967295");
                    Console.ForegroundColor = ConsoleColor.Black;
                }
                catch (ArgumentNullException)
                {
                    Console.ForegroundColor = ConsoleColor.Red;
                    Console.WriteLine("Napotkano koniec strumienia");
                    Console.ForegroundColor = ConsoleColor.Black;
                }
                Console.WriteLine("Spróbuj jeszcze raz");
            }
        }

Dlaczego po prostu nie zwrócisz wartości przez return? void PodajLiczbę -> int PodajLiczbę.
Nie powtarzaj się.

        static void WyświetlBłąd( string komunikat )
        {

            Console.ForegroundColor = ConsoleColor.Red;
            Console.WriteLine(komunikat);
            Console.ForegroundColor = ConsoleColor.Black;
        }

Dzięki temu Twój kod staje się czytelniejszy, łatwiejszy do edycji i krótszy. Nazwa "podaj liczbę" nie odzwierciedla zawartości funkcji.

"int val;" nie wiadomo co oznacza. "public int v" po co to komu i co to znaczy? Skorzystaj Zadeklaruj v jako readonly i tyle.

Jeszcze raz spojrzałem, na kod to okazuje się że ten konstruktor, v, val są nieużywane więc je usuń. W main deklarujesz na początku zmienne które będziesz używał w for, po co?... Taki zachowaniem sugerujesz, że powinny być gdzieś wcześniej zainicjowane lub użyte co jest nieprawdą. Zrób normalne for ( int i = 0; i < liczba; i++), no ale czym jest "liczba"? liczbaElementów? Albo jeszcze lepiej tablica.Length. Poza tym wewnątrz tej pętli masz inną która jest całkowicie zbędna.

Najlepiej podziel Main na mniejsze metody typu void Inicjacja (ustawienia konsoli), int[] WczytywanieElementów, int[] Sort (niech zwraca wynik w nowej tablicy), void WyświetlTablicę.

Przede wszystkim dziękuję za uwagi. Teraz komentarz:

zxxcxzcxzzx napisał(a):

Jeszcze raz spojrzałem, na kod to okazuje się że ten konstruktor, v, val są nieużywane więc je usuń.

Też chętnie bym tak zrobił, ale postawione przede mną zadanie mówi wyraźnie, by skorzystać z podanej klasy Element.

zxxcxzcxzzx napisał(a):

W main deklarujesz na początku zmienne które będziesz używał w for, po co?... Taki zachowaniem sugerujesz, że powinny być gdzieś wcześniej zainicjowane lub użyte co jest nieprawdą. Zrób normalne for ( int i = 0; i < liczba; i++), no ale czym jest "liczba"? liczbaElementów? Albo jeszcze lepiej tablica.Length.

Sugerujesz, że mam ich w ogóle nie deklarować, czy deklarować w innym miejscu?

zxxcxzcxzzx napisał(a):

Poza tym wewnątrz tej pętli masz inną która jest całkowicie zbędna.

Słuszna uwaga.

                try
                {
                    liczba = int.Parse(str);
                    break;
                }
                catch (FormatException)
                {
                    Console.ForegroundColor = ConsoleColor.Red;
                    Console.WriteLine("Wprowadzono liczbę w złym formacie");
                    Console.ForegroundColor = ConsoleColor.Black;
                }

spróbuj:

if (!int.TryParse(str, out liczba))
{
  // niepoprawna liczba
}

Xiuthechutli napisał(a):

Zgodnie z zadaniem elementami tablicy powinny być obiekty klasy Element.

A u Ciebie cały program jest wywołaniem metody Main z klasy Element.
Zdaje się, że Twoja klasa Element powinna wyglądać mniej więcej tak (przy czym konstruktor jest w opcjonalny):

public class Element
    {
        public int Val { get; set; }

        public Element(int e)
        {
            Val = e;
        }
    }

Tylko że nie mogę właśnie zmienić podanej klasy. Jak rozumiem, pole val jest prywatne i nie będzie do niego bezpośredniego dostępu. Jeśli dobrze pamiętam, dostęp do takiego pola możliwy jest przez metodę lub przez właściwość (u mnie v).

class Program
    {
        
        static void Main(string[] args)
        {
            var rand = new Random();
            Element[] elements = new Element[rand.Next(4, 8)];

            for (int i = 0; i < elements.Length; i++)
                //Uzupełnienie losowymi elementami z użyciem konstruktora
                elements[i] = new Element(rand.Next(100));
            

            Console.WriteLine("Przed sortowaniem:");
            elements.ToList().ForEach(x => Console.WriteLine(x.Val));

            Console.WriteLine("Po sortowaniu:");
            elements.OrderBy(e => e.Val).ToList().ForEach(e => Console.WriteLine(e.Val));
            
            Console.ReadLine();
        }
    }

    public class Element
    {
        public int Val { get; private set; }//To jest właściwość, pola nie ma w kodzie tworzy go kompilator.
        public Element(int v)
        {
            Val = v;
        }
    }

Istnieje klasa Element, w której jest prywatne pole uzupełniane w konstruktorze przez właściwość Val (Pole jest generowane przez kompilator).
Poczytaj to: http://4programmers.net/Forum/C_i_.NET/231592-jak_to_jest_z_wlasciwoscia_get_i_set

    class Program
    {
        static void Main(string[] args)
        {
            UstawStyle();
            Element[] elementy = WczytajDaneZKonsoli();
            Posortuj(elementy);
            WyswietlDaneWKonsoli(elementy);
        }

        static void UstawStyle()
        {
            Console.Title = "Projekt";
            Console.BackgroundColor = ConsoleColor.White;
            Console.ForegroundColor = ConsoleColor.Black;
            Console.Clear();
        }
    }

    class Element
    {
        public int Wartosc;

        public Element(int wartosc )
        {
            Wartosc = wartosc;
        }
    }

Masz za zadanie wczytac elementy z konsoli, później je posortować i wyświetlić. Dzięki zrobieniu wielu małych metod widać poszczególne etapy i łatwo zmodyfikować główny kod...

        static void Main(string[] args)
        {
            UstawStyle();
            Element[] elementy = WczytajSztywneDane();
            Posortuj(elementy);
            WyswietlDaneWKonsoli(elementy);
            Console.ReadLine();
        }

        static Element[] WczytajSztywneDane()
        {
            return new Element[]
            {
                new Element(1),
                new Element(5),
                new Element(10),
                new Element(4),
                new Element(3),
            };
        }

Dostajemy tablicę elementów, więc w sortowaniu trzeba porównać element.Wartosc...

        static void Posortuj(Element[] tablica)
        {
            for (uint i = 1; i < tablica.Length; i++)
            {
                uint j = i;
                int buf = tablica[j].Wartosc;
                while ((j > 0) && (tablica[j - 1].Wartosc > buf))
                {
                    tablica[j].Wartosc = tablica[j - 1].Wartosc;
                    j--;
                }
                tablica[j].Wartosc = buf;
            }
        }

W sumie kod wygląda nastepująco:

using System;

namespace ConsoleApplication13
{
    class Program
    {
        static void Main(string[] args)
        {
            UstawStyle();
            Element[] elementy = WczytajSztywneDane();
            Posortuj(elementy);
            WyswietlDaneWKonsoli(elementy);
            Console.ReadLine();
        }

        static void UstawStyle()
        {
            Console.Title = "Projekt";
            Console.BackgroundColor = ConsoleColor.White;
            Console.ForegroundColor = ConsoleColor.Black;
            Console.Clear();
        }

        static Element[] WczytajDaneZKonsoli()
        {
            throw new NotImplementedException();
        }

        static Element[] WczytajSztywneDane()
        {
            return new Element[]
            {
                new Element(7),
                new Element(5),
                new Element(10),
                new Element(4),
                new Element(3),
            };
        }

        static void Posortuj(Element[] tablica)
        {
            for (uint i = 1; i < tablica.Length; i++)
            {
                uint j = i;
                int buf = tablica[j].Wartosc;
                while ((j > 0) && (tablica[j - 1].Wartosc > buf))
                {
                    tablica[j].Wartosc = tablica[j - 1].Wartosc;
                    j--;
                }
                tablica[j].Wartosc = buf;
            }
        }

        static void WyswietlDaneWKonsoli(Element[] elementy)
        {
            Console.WriteLine("Posortowane elementy: ");
            for (int i = 0; i < elementy.Length; i++)
            {
                Console.WriteLine("Element [{0}] = {1}", i, elementy[i].Wartosc);
            }
        }
    }

    class Element
    {
        public int Wartosc;

        public Element(int wartosc )
        {
            Wartosc = wartosc;
        }
    }
}

Ty zaimplementuj wczytywanie z konsoli i zmień Sortowanie tak, żeby było niedestruktywne (czyli nie zmieniało tablicy, a zwracało świeżą).