Kod do oceny

0

Ktoś mi napisał że mam wszystko za bardzo rozrzucone, nie studiuje informatyki ani nie pracuje jako programista wiec nie mam możliwości weryfikacji. Jakby ktoś wskazał mi błedy i rzeczy do poprawy to nie żałował bym plusów :P

To jest kontrolka robiąca jako plansza do warcab, załóżmy że nawet działa. PawnMap to jest mapa Pionków nakładka na tablice 2d, Queen, Pawn, EmptyCell to pionki na wspólnym interfejsie, Move to obiekt wykonujący ruch na PawnMap,"From" to pole z którego ruch się zaczyna, a "To" to jest pole na którym się kończy. Przechowuje co prawda jeszcze jakieś informacje ale ale z punktu widzenia grafiki to nie istotne - może przez to wypadało by go czyms zastąpić nwm, jest jak jest :P. To że używam 2 różnych rodzaj klas Point wynika z tego że MinMax2 i MinMax2 są z innego projektu i tam tak jest. To chyba wszystko:).
Plansza ma wymiar 8x8 pól stąd ta magiczna 8.

@Shalom @somekind @katelx

using MinMax2;
using MinMax2.Pawns;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows;
using System.Windows.Controls;
using System.Windows.Data;
using System.Windows.Documents;
using System.Windows.Input;
using System.Windows.Media;
using System.Windows.Media.Imaging;
using System.Windows.Navigation;
using System.Windows.Shapes;
using PointSource = System.Drawing;

namespace WarcabyGrafika
{
    /// <summary>
    /// Interaction logic for Plansza.xaml
    /// </summary>
    public partial class Plansza : UserControl
    {
        private readonly BitmapImage WHITE_QUEEN= new BitmapImage(new Uri("pack://application:,,,/img/HetmanB.png"));
        private readonly BitmapImage WHITE_PAWN = new BitmapImage(new Uri("pack://application:,,,/img/PionekB.png"));
        private readonly BitmapImage BLACK_PAWN = new BitmapImage(new Uri("pack://application:,,,/img/PionekC.png"));
        private readonly BitmapImage BLACK_QUEEN = new BitmapImage(new Uri("pack://application:,,,/img/HetmanC.png"));

        IList<Move> _posibleMoves = new List<Move>();
        Image[,] _imgs = new Image[8, 8];
        private Image _movingImg = new Image();
        PawnMap _lastMove = new PawnMap(8);
        bool _isMoving = false;
        PointSource.Point _from = new PointSource.Point(-1, -1);

        public event Action<PointSource.Point, PointSource.Point> MoveSelected;

        public Plansza()
        {
            InitializeComponent();
            _imgs = InitImages();
            _from = new PointSource.Point(-1, -1);
            this.MouseLeave += BreakMoving;
            this.PreviewMouseLeftButtonUp += StopMoving;
            this.PreviewMouseMove += PawnMoved;
            _movingImg.Source = WHITE_PAWN;
            _movingImg.Opacity = 0.7;
            this.playGround.Children.Add(_movingImg);
        }
        #region Initalize private
        private Image[,] InitImages()
        {
            Image[,] imgs = new Image[8, 8];
            for (int y = 0; y < 8; y++)
            {
                for (int x = 0; x < 8; x++)
                {
                    imgs[x, y] = new Image();
                    imgs[x, y].PreviewMouseDown += StartMoving;
                    SetPostionOnMap(imgs[x, y], x, y);
                    playGround.Children.Add(imgs[x, y]);
                }
            }
            return imgs;
        }
        private void SetPostionOnMap(FrameworkElement imgs, int x, int y)
        {
            const int marginX = 3;
            const int marginY = 3;
            imgs.Margin = new Thickness(marginX, marginY, 0, 0);
            Canvas.SetLeft(imgs, FieldWight * x);
            Canvas.SetTop(imgs, FieldHeight * y);
        }
        #endregion
        public double FieldWight { get { return playGround.Width / 8; } }
        public double FieldHeight { get { return playGround.Height / 8; } }
        public void UpDate(PawnMap map, IList<Move> moves)
        {
            if (map.IsNull() || moves.IsNull())
                throw new ArgumentNullException("Map and null list can not neither be null or empty");
            _posibleMoves = moves;
            DrawMap(map);
            _lastMove = map.Copy();
        }
        private void DrawMap(PawnMap map)
        {
            for (int y = 0; y < 8; y++)
            {
                for (int x = 0; x < 8; x++)
                {
                    if (map.GetAt(x, y) != _lastMove.GetAt(x, y))
                    {
                        _imgs[x, y] = DrawPawn(_imgs[x, y], map.GetAt(x, y));
                    }
                }
            }
        }
        private Image DrawPawn(Image img, IMan man)
        {
            if (man == Queen.WhiteInstance)
                img.Source = WHITE_QUEEN;
            else if (man == Queen.WhiteInstance)
                img.Source = BLACK_QUEEN;
            else if (man == Pawn.WhiteInstance)
                img.Source = WHITE_PAWN;
            else if (man == Pawn.BlackInstance)
                img.Source = BLACK_PAWN;
            else if (man == PawnMap.EmptyCell)
            { img.Visibility = System.Windows.Visibility.Collapsed; return img; }
            else
                throw new NotImplementedException("Unknow pawn Type");
            
            img.Visibility = System.Windows.Visibility.Visible;
            img.UpdateLayout();
            return img;
        }
        #region Mouse moving and selecting
        private void StartMoving(object sender, MouseButtonEventArgs e)
        {
            var p = GetPawnFromCoords(e.GetPosition(this.playGround));
            if (!_lastMove.IsInRage(p))
                return;
            if (_posibleMoves.Any(m => m.To.Equals(p)))
                return;
            _isMoving = true;

            _movingImg = DrawPawn(_movingImg, _lastMove.GetAt(p));
            _from = p;
        }
        private void StopMoving(object sender, MouseButtonEventArgs e)
        {
            var targetPoint = GetPawnFromCoords(e.GetPosition(playGround));
            bool isMovePosible =  _posibleMoves.Any(m => m.From.Equals(_from) && m.To.Equals(targetPoint));
            if (isMovePosible)
            {
                if (MoveSelected != null)
                    MoveSelected.Invoke(_from, targetPoint);
            }
            BreakMoving(sender, e);
        }
        private void BreakMoving(object sender, MouseEventArgs e)
        {
            _isMoving = false;
            _movingImg.Visibility = System.Windows.Visibility.Collapsed;
            _from = new PointSource.Point(-1, -1);
            Cursor = Cursors.Arrow;
        }

        private void PawnMoved(object sender, MouseEventArgs e)
        {
            if (_isMoving)
            {
                _movingImg.Visibility = System.Windows.Visibility.Visible;
                Canvas.SetLeft(_movingImg, e.GetPosition(playGround).X - 0.5 * FieldWight);
                Canvas.SetTop(_movingImg, e.GetPosition(playGround).Y - 0.5 * FieldHeight);
            }
            Cursor = SelectCursorView(e.GetPosition(this.playGround));
        }

        private Cursor SelectCursorView(Point point)
        {
            var p = GetPawnFromCoords(point);

            if (!_isMoving && _posibleMoves.Any(m => m.From.Equals(p)))
                return Cursors.Hand;
            else if (_isMoving && _posibleMoves.Any(m => m.From.Equals(_from) && m.To.Equals(p)))
                return Cursors.Hand;
            else if (_isMoving)
                return Cursors.No;
            else
                return Cursors.Arrow;
        }

        private PointSource.Point GetPawnFromCoords(Point coords)
        {
            int x = 7 - (int)((playGround.Width - coords.X) / FieldWight);
            int y = 7 - (int)((playGround.Height - coords.Y) / FieldHeight);
            return new PointSource.Point(x, y);
        }
        private void GetPawnFromCoords(Point coords, out int x, out int y)
        {
            x = (int)(coords.X / FieldWight);
            y = (int)(coords.Y / FieldHeight);
        } 
        #endregion
    }
}

 
2

Co do "magicznej ósemki" to może w takim razie warto stworzyć zmienną boardSize? Zamiast pisać komentrze do kodu praktycznie zawsze można użyć odpowiednich nazw zmiennych, które wszystko wyjaśnią. Poza tym masz część nazw po polsku, a część po angielsku to nie wygląda dobrze. Masz też niespójne formatowanie (spacje, entery etc.).

3

jesli tylko program dziala to gratuluje, nie wyglada to tak zle :)
przekartkuj sobie ksiazke 'clean code' i jakies standardy nazewnictwa i kodowania w c#

pare kwestii rzucajacych sie w oczy:

  • masa logiki na kontrolce (a powinna byc wydzielona do osobnych klas)
  • za duzo zmiennych mutowanych w roznych miejscach, takie cos bardzo szybko wymyka sie spod kontroli
  • zduplikowane kawalki kodu, zamiast robic copy-paste po prostu napisz metode
  • zasoby (grafika) powinny zdeklarowane gdzie indziej (resources?), +TAKICH_NAZW_SIE_NIE_STOSUJE
  • mix pl/en (nie jakis hardkorowy ale zawsze)
  • klasa powinna byc sealed, wiekszosc pol readonly
  • ta osemka jest do d**y
  • pokreslanie na poczatku nazwy pola jest zbedne
  • zdecyduj sie gdzie inicjalizowac pola, albo inline albo w konstruktorze (preferowane), btw niektore sa inicjalizowane i tu i tu
  • publiczna metoda UpDate - robisz kopie map, ale moves juz olales, w ogole ta metoda to troche dziura jest bo potem zakladasz ze jest 8x8
  • zrob sobie slownik tych pionkow i pol a nie takie else/ify produkujesz, enum moze tez miec sens. serio moze byc tak ze bedzie nieznany pionek? ;)
  • jednoliterowe zmienne to zly pomysl (poza for)
  • przeladuj operator == zamiast uzywac Equals dla punktow
  • nie uzywaj #region (ok - to akurat bardzo subiektywne, ale nienawidze ich)
  • nawet jak masz 1 linijke ifa to i tak uzywaj klamer
1

Pionek powinien być osobną klasą. Powinien wiedzieć czym jest (czy zwykły pionek, czy królowa) i jaki ma kolor. Idealnie by było, gdyby też sam się rysował.

0

Dzięki wielkie za wypunktowanie :).
CleanCode i refactoring i niby przewertowałem ale tych książek nie wystarczy tylko przewertować np. to z polami readonly i słownikiem super pomysł :), sam bym na takie rozwiązanie za szybko nie wpadł :) a słowo seald tylko raz użyłem.
Nazewnictwo i te szczegóły jak inicjalizacja przyjmuje z pokorą :P.
Nie rozumiem czemu mam za dużo logiki, wydaje mi się ze tylko niewinnie sprawdzam czy i jaki kursor wyświetlić i jak wyświetlać przesuwany pionek, a resztę biorę z zewnątrz.
Co do nieznanego pionka teoretycznie w testach mam jeszcze jedną klasę żeby wymuszać ruchy na ai('ai' alfabeta i minmax nic specjalnego) i nie jest ona sigletonem, a praktycznie za łatwo sobie strzelić w kolano nie wyrzucając wyjątku bo przecież to się nie mogło stać:P(śmiesznie to brzmi patrząc na zbugowany rozmiar mapy^^).
Kopie mapy robię żeby móc porównać, co się zmieniło od poprzedniego razu i podmienić tylko te obrazki co się zmieniły, przy zamianie wszystkich czasem nie chodzi płynnie. Z ruchami nie mam takiej potrzeby.
Pionki nie wyświetlają grafiki celowo bo miały być tylko na potrzeby ai, nie chce historii nowoczesnej opowiadać ale miałem zrobić dla kogoś algorytmy a, on podpiąć je do swojej grafiki winformsach(stąd 2 rodzaje punktów i zmienny rozmiar mapy). No i plan był taki żeby broń boże nie mieszać niczego z grafiką bo nie działała i jeszcze byłoby na mnie :P. A teraz pisze sam dla siebie:P

1

z ta logika chodzi mi o to ze robisz sporo obliczen i warunkow na formie, zalozmy ze z jakis powodow chcialbys to teraz miec w winformsach albo pod konsola ;) to musialbys to wsyzstko przepisac zeby dostosowac pod UI. dodatkowo - zakladajac ze rozwijasz to i dodajesz nowe efekty wizualne, albo np piszesz szachy zamiast warcabow to ta forma stala by sie dosc monstrualna.
wydziel sobie jakis model niezalezny od widoku, poczytaj o mvc, mvp, mvvp czy co tam teraz jest modne dla GUI ;)

0

@katelx @Shalom
Tak przy okazji. Powiedzmy że mam taką klasę. Gdybym usunął pole readonly map i przekazał bym je jake parametr to liczba do metod to było to wydaje mi się zeby było by lepiej, bo nie trzeba byłoby się martwić usuwaniem takiego obiektu po zakończeniu gry ani bawi się ze slide efektami w postaci zmiany stanu mapy w innej cześci kodu.
Zakładając że pomysł z przekazaniem mapy jako parametr jest dobry można to samo zrobić z kolorem, tylko problem jest taki że w takim przypadku w metody prywatę miałyby duzo parametów. Żeby to objeść można napisać strukturę AkrualnaTura z kolorem gracza i kopią planszy. Wszystko fajnie pięknie trzeba pisać kropkę ale jest mało jest mało parametrów, i nie ma slideEfektów tylko problem jest taki że ze klasa stała sie zestawem funkcji i nie wiem czy tak powinno być(?). Nie ogarniam tego :(

 
class HumanPlayer
{
    private readonly Color color;
    private readonly PawnMap map;
    public HumanPlayer(PawnMap map ,Color color);
    public Move WczytajRuch( Gui gui );
    public Ienumerable<Point> DajPolaZktórychMozliwyJestRuch();
    public Ienumerable<Point> DajPolaNeKtóreMozliwyJestRuch(point point); // gdzie sie mozna ruszyc ze wskazanego
    // metody pywatne do zrealizowania celu();
}
0

@topik92 mysle ze player nie powinien miec w sobie mapy, zestawu ruchow a jedynie swoja pozycje. pewnie bym zaczela od zdefiniowania takich klas na poczatek:

  • pionki
  • mapa trzymajaca puste pola i pionki
  • gracz trzymajacy swoje punkty
  • jakis widok
  • silnik ktory to wszystko spina razem (aktualizuje widok, mape i graczy)

Zarejestruj się i dołącz do największej społeczności programistów w Polsce.

Otrzymaj wsparcie, dziel się wiedzą i rozwijaj swoje umiejętności z najlepszymi.