Kod do oceny

Odpowiedz Nowy wątek
2016-04-26 20:14
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
    }
}
 
edytowany 2x, ostatnio: topik92, 2016-04-26 20:24
edycja postu nie wysyła powiadomień z @ - Shalom 2016-04-26 20:18
Powiadomienia były przed edycją, czyli dotarły? dobrze rozumiem? - topik92 2016-04-26 20:20
a faktycznie ;] - Shalom 2016-04-26 20:48

Pozostało 580 znaków

2016-04-26 21:34
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.).

Pozostało 580 znaków

2016-04-27 16:16
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
Z tymi ifami jednolinijkowymi to dwie szkoły są. - xeo545x39 2016-04-27 23:23
Dwie szkoły - jedna bezsensowna, a druga ta, która używa klamer. - somekind 2016-04-28 00:12
Zależy od konkretnego przykładu. Jak dla mnie krótki warunek i to co w ifie może być w jednej linijce, bez entera. Ale po tym odstęp 1 pustej linjki. Reszta do kitu. Ale ja sam używam w 90% klamer, bo mimo wszystko czytelniej tylko czasem po prostu nie warto wydłużać kodu i napisać w jednej, o ile jest to jakiś prosty kod np. wywołanie metody i koniec. - xeo545x39 2016-04-28 14:29
@xeo545x39 czytelnosc na tym cierpi (ok, to jest subiektywne), spojnosc, a z takich konkretow - zalozmy ze chcesz wykomentowac to wywolanie metody, gdy masz klamry to po prostu robisz // metoda(); i problem z glowy, bez klamer jest wiecej pieprzenia, jak zapomnisz wstawic ; to troche ci to napaskudzi - katelx 2016-04-28 15:51

Pozostało 580 znaków

2016-04-27 17:07
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ł.

Pionek jest osobną klasą i królowa jest osobną klasą, puste pole jako specjal case jest osobną klasą na tym wysztkie sa na tym samym interfejsie. Nie widać tego tutaj, ale ich głównym zadaniem jest żeby wiedziały jak się ruszać na podanej planszy, a zmartwieniem grafiki jest jak ma je wyświetlić. Nie sprawdzam po kolorze i rodzaju bo po co skoro mam tylko 5 obiektów a warunków na rodzaj kolor i wolne pole powinno być 9 licząc i/lub jako warunek. - topik92 2016-04-27 20:43
To błąd, że królowa jest osobną klasą, chyba że dziedziczy po pionku. Wtedy takie zachowanie jest ok. Jeśli jednak królowa i pionek nie są ze sobą powiązane, to jest źle. - Juhas 2016-04-28 16:43
Są powiązane interfejsem, dlaczego jest złe że nie dziedziczą bez pośrednio po sobie? i dlaczego koniecznie po pionku? jeśli tak się zastanowić to ruch pionka jest w pewnym szczególnym przypadkiem ruchu królowej, w którym ograniczamy długość wyszukiwania ruchów do jedenego pola. Patrząc w ten sposób potrzeba tylko jednej klasy a to czy figura będzie pionkiem, królem, damką czy nieruchomą atrapą zależeć będzie od parametru w konstruktorze. nie zrobiłem tak bo na to nie wpadłem, ale i tak nie rozumiem czemu królowa ma dziedziczyć po pionku dla 3 podobnych linii kodu. - topik92 2016-04-28 20:22

Pozostało 580 znaków

2016-04-27 20:31
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

edytowany 1x, ostatnio: topik92, 2016-04-27 20:39

Pozostało 580 znaków

2016-04-28 16:05
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 ;)

w sumie masz racje, dzieki:P - topik92 2016-04-28 20:30

Pozostało 580 znaków

2016-05-04 11:18
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();
}

Pozostało 580 znaków

2016-05-04 16:46
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)

Pozostało 580 znaków

Odpowiedz
Liczba odpowiedzi na stronę

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