Prośba o sprawdzenie kodu

0

Witam. Chcę zrobić program rozwiązujący problem komiwojażera. Jestem na etapie, gdzie mam już tablicę z obliczonymi długościami tras pomiędzy miastami. Chciałbym byście sprawdzili mi poprawność tego kodu. Wiem, że może być w nim wiele błędów jakich nie powinno się robić i nie chcę ich później popełniać :)

Kod z plikiem Komi.cs

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Windows.Forms;
 
namespace Mini_Paint
{
    public partial class frmMiniPaint : Form
    {
 
        private Graphics g;
        int j, i; 
        public int l; // liczba miast
        int[,] wspMiast = new int[50, 3];//tablica do zapisania współżędnych miast
 
        public frmMiniPaint()
        {
            InitializeComponent();
            imgObrazek.Image = new Bitmap(600, 530);
        }
 
        private void frmMiniPaint_Load(object sender, EventArgs e)
        {
 
       } 
        private void imgObrazek_MouseDown(object sender, MouseEventArgs e)
        {
            int y = 14;//wielkość punktów na mapie
            if (g == null) return;
            if (e.Button == MouseButtons.Left) 
            {
                g.FillEllipse(Brushes.Black, e.X-6, e.Y-6, y, y);
                imgObrazek.Refresh();
 
                    int xx = Cursor.Position.X;//przypisanie pozycji myszki do zmiennych
                    int yy = Cursor.Position.Y;
 
//------------------------------pętla zapisująca numer miasta/punktu, oraz pozycje miast/punktów do tablicy
                        for (j=0; j<3; j++)
                        {
                            if (j == 0)
                            {
                                wspMiast[i, j] = i+1;
                            }
                            else if (j == 1)
                            {
                                wspMiast[i, j] = xx;
                            }
                            else if (j == 2)
                            {
                                wspMiast[i, j] = yy;
                            }
                        }
//-------------------------------------------------------
                       i = i + 1;//licznik ilości miast/kliknięć na mapie
                       l = i;
            }
        }
 
        private void imgObrazek_MouseMove(object sender, MouseEventArgs e)
        {
 
        }
 
        private void imgObrazek_MouseEnter(object sender, MouseEventArgs e)
        {
 
        }
 
        private void otwórzToolStripMenuItem_Click(object sender, EventArgs e)
        {
//-----------wczytanie obrazka z wyglądem mapy            
            OpenFileDialog dialog = new OpenFileDialog();
            dialog.ShowDialog();
            if (dialog.FileName == "") return;
            imgObrazek.Load(dialog.FileName);
            g = Graphics.FromImage(imgObrazek.Image);
//---------------------           
        }
 
        private void imgObrazek_Click(object sender, EventArgs e)
        {
 
        }
 
        private void button1_Click(object sender, EventArgs e)
        {
//---------------pętle wyśwoietlające pozycje miast
            for (i = 0; i < 50; i++ )
            {
                for (j = 0; j < 3; j++)
                {
                    Console.Write("i=" +i+ " ,j=" +j+ " - ");
                    Console.Write(wspMiast[i, j]);
                    Console.WriteLine(" ");
                }
            }
//--------------------------
            Console.WriteLine(l); //wyświetlenie ilości miast/punktów na mapie
            Console.WriteLine("tablica z odległościami");
 
            int[,] odlMiast = new int[l+1, l+1];//tablica z odległościami między miastami
 
//---------pętla wypełniająca tablicę z odległościami pomiędzy miastami
            for (i = 0; i < l+1; i++)
            {
                for (j = 0; j < l+1; j++)
                {
                    if (i == 0)
                    {
                        odlMiast[i, j] = j;
                    }
                    else if (j == 0)
                    {
                        odlMiast[i, j] = i;
                    }
                    else
                    {
                        odlMiast[i,j] = Convert.ToInt32(Math.Sqrt((wspMiast[i-1, 1] - wspMiast[j-1, 1]) * (wspMiast[i-1, 1] - wspMiast[j-1, 1]) + ((wspMiast[i-1, 2] - wspMiast[j-1, 2]) * (wspMiast[i-1, 2] - wspMiast[j-1, 2]))));
                    }
                }
            }
//-------------------------------
//--------------wyświetlanie tablicy z odległościami
            for (i = 0; i < l+1; i++)
            {
                for (j = 0; j < l+1; j++)
                {
                    Console.Write("i=" + i + " ,j=" + j + " - ");
                    Console.Write(odlMiast[i, j]);
                    Console.WriteLine(" ");
                }
            }
//-----------------------
        }
    }
} 

Kod z pliku Program.cs

 
using System;
using System.Collections.Generic;
using System.Linq;
using System.Windows.Forms;
 
namespace Mini_Paint
{
    static class Program
    {
 
        /// <summary>
        /// The main entry point for the application.
        /// </summary>
        [STAThread]
        static void Main()
        {
 
            Application.EnableVisualStyles();
            Application.SetCompatibleTextRenderingDefault(false);
            Application.Run(new frmMiniPaint());
        }
    }
}

Jeśli ktoś woli sprawdzić działanie, to oczywiście prześlę program na PW lub maila.
Potrzebuję również w nowej klasie, która znajduje się w innym pliku xxxx.cs tablicę odlMiat, lecz nie wiem jak ją pobrać.
Byłbym wdzięczny za wszelkie porady, oraz krytykę.

0

Błędów "rzeczowych" - czyli potencjalnego wywalenia się programu - nie widzę. Co nie znaczy że ich nie ma, bo nie ma czegoś takiego jak program bez błędów :)

// Plik Program.cs omijam bo nic tam nie dodawałeś

Co do stylu jednak mam kilka zastrzeżeń. Większość z nich to szczegóły, ale nic poważniejszego nie widzę.

  • usingi - w sumia można sporo z nich wywalić - ComponentModel, Data, Linq, Text a nawet Collections.Generic wyglądają na nieużywane. Absolutnie nic to nie zmienia w programie, ale niepotrzebnie zajmują linie kodu - ja zawsze staram się wywalać usingi kiedy nie używam nic z przestrzeni nazw.

// - namespace Mini_Paint - kwestia gustu, ale ogólnie podkreślenia brzydko wyglądają.

  • frmMiniPaint - Dobry pomysł żeby zmienić domyślne Form1! Gorzej że ogólnie przyjętą zasadą jest żeby zaczynać nazwę klasy z dużej litery - jeśli zostaniesz programistą z zawodu i będziesz pracował w zespole takie rzeczy nie będą ci wybaczone ;)

  • zmienne klasy - jeśli używasz modyfikatorów private rób to konsekwentnie, przed wszystkimi zmiennymi. Pomijam że są nazwane po polsku.

  • zmienne publiczne są zdecydowanie odradzane. Dopóki projekt jest mały OK, ale ogólnie to złe przyzwyczajenie.

  • współ -> rz <- ędne!

  • konsola w aplikacji okienkowej?

  • puste metody obsługi zdarzeń? Niepotrzebnie, tylko marnują pracę procesora (i poszerzają kod)

  • int y = 14 - tzw "Magic number" - powinno się dążyć do unikania takich stałych liczbowych w kodzie. Tytaj o tyle dobrze że jest to ładnie skomentowane. Ogólnie ja byn z tego zrobił stałą.

  • Antywzorzec "MagicButton" - dużo logiki aplikacji zawartej w metodzie obsługi zdarzenia. Teoretycznie w bardzo małych projektach to nie problem, ale to bardzo złe przyzwyczajenie.

  • posiekanie kodu blokami komentarzy - nie zrozum źle, komentowanie to dobra rzecz. Ale czemu wyrównujesz komentarze całkiem do lewej strony? IMO to trochę zaburza widok "hierarchiczny".

0

Dziękuję za zainteresowanie, oraz uwagi. Kilka z nich już przetrawiłem i poprawiłem swój program.
Jeśli chodzi o konsolę w tej aplikacji, to mam ją, by monitorować wartości poszczególnych zmiennych i tablic w celu sprawdzenia poprawności działania. Tablice pewnie w DataGrid-ie mógłbym sobie wyświetlać, ale nie wiem jeszcze jak.

Pisząc o "Magic number" miałeś na myśli to, by dać całe obliczenia do osobnej klasy?

Jeśli chodzi o komentarze, to faktycznie masz rację, trochę to nawet dziwnie wygląda, poprawię się w tej kwestii.
Kolejne uwagi będą mile widziane.

0

Zgadzam się z przedmówcą, do tego dołożę swoje trzy grosze:

  1. Nazwy zmiennych: unikaj jednoliterowych nazw zmiennych, staraj się nadawać nazwy, które coś oznaczają, zamiast pisać
public int l; // liczba miast 

napisz private int liczbaMiast;

 komentarz jest w tym przypadku zbędny, bo po nazwie zmiennej wiadomo z czym mamy do czynienia. Jak twój kod znacznie się rozrośnie to może stać się kłopotliwe przypominanie sobie co ta zmienna "l" znaczyła. Staraj przyzwyczajać się do nazywania zmiennych po angielsku, przyznasz że lepiej wygląda: <code class="csharp">private void pictureBoxMap_MouseDown(object sender, MouseEventArgs e) 

niż zlepek polsko-angielski: private void imgObrazek_MouseDown(object sender, MouseEventArgs e)

 I znowu nazwa zmiennej(domyśliłem się, że obrazek wyświetla coś w rodzaju mapy). Konwencje nazywania zmiennych: http://msdn.microsoft.com/en-us/library/xzf533w0%28v=vs.71%29.aspx
2. Rozbudowane if'y: jeszcze trzy warunki nastepujące po sobie to nic złego, ale jeżeli byłoby ich więcej zastosuj po prostu switch, wtedy else możesz zastąpić przecież słowem "default:".
3. Może się czepiam, ale zamiast 
```csharp
i = i + 1;

używaj i++;

to to samo, ale imo leciutko czytelniejsze
4. To o czym wspomniał MSM: nie pisz długich metod w obsłudze eventów, zamiast tego utwórz sobie oddzielną metodę, a w handlerze tylko ją wywołuj
5. Zamiast konsoli do wyświetlania kontrolnych danych używaj np. klas Debug i Trace: http://support.microsoft.com/kb/815788
0

Kolejne błędy poprawiłem.
Teraz użyłem klasy Debug do wyświetlania kontrolnych danych i szczerze mówiąc, to jakoś nie widzę różnicy pomiędzy Console. Muszę przyznać też, że ta klasa ma kilka opcji, które na pewno mi się przydadzą :)

Teraz chcę tak przerobić kod, by metody mieć w oddzielnej klasie, nie w obsłudze eventów, ale mi to nie wychodzi.
Na przykład chcę przenieść kod odpowiedzialny za obliczanie odległości pomiędzy miastami do osobnej metody w innej klasie, ale nie wiem jak pobrać dane (np. wspMiast, liczbaMiast, które w powyższym kodzie jest jako l ), które są generowane podczas klikania na mapę.

1 użytkowników online, w tym zalogowanych: 0, gości: 1