Pomodoro | WinForms | Code Review

Question

Witam!
Proszę Cię o opinie i porady co do kodu tej aplikacji.
Jest to prosta aplikacja typu Pomodoro napisana w Windows Forms.

Gitchub: https://github.com/B0ZIA/Pomodoro

 

Jeśli nie masz aż tyle czasu lub chęci to proszę Cię o chociaż zerknięcie na poniższe klasy"
 

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;


namespace Pomodoro
{
    public abstract class Timer
    {
        protected static System.Windows.Forms.Timer clock;
        private int _currentTime = 1800;    //default 30 min
        private static bool exitFlag = false;

        

        public virtual void Tick(Object myObject, EventArgs myEventArgs)
        {
            _currentTime -= 1;
            TimeSpan time = TimeSpan.FromSeconds(_currentTime);
            Pomodoro.Instance.GetTimeLabel().Text = time.ToString(@"mm\:ss");

            if (TimeoutScreen.time != null)
            {
                TimeoutScreen.time.Text = time.ToString(@"mm\:ss");

                if (_currentTime == 0)
                    TimeoutScreen.Instance.Close();
            }

            if (_currentTime == 60)
                LastMinute();

            if (_currentTime == 0)
                Timeout();
        }

        public abstract void Timeout();

        public abstract void LastMinute();

        protected void StartClock(int seconds)
        {
            if (clock != null)
                clock.Stop();
            clock = new System.Windows.Forms.Timer();
            _currentTime = seconds;

            for (int i = 0; i <= seconds; i++)
            {
                clock.Tick += new EventHandler(Tick);

                clock.Interval = 1;
                clock.Start();

                while (exitFlag == false)
                {
                    Application.DoEvents();
                }
            }
        }
    }
}

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using System.Media;

namespace Pomodoro
{
    public class PomodoroTimer : Timer
    {
        public static PomodoroTimer Instance;
        public const int BREAK_TIME = 300;    //5 min
        public const int WORK_TIME = 1800;    //30 min

        State state;
        State stateBeforeLastPause;

        static PomodoroNavigation pomodoroNavigation;



        public PomodoroTimer(PomodoroNavigation pomodoroNavigation)
        {
            Instance = this;
            PomodoroTimer.pomodoroNavigation = pomodoroNavigation;
        }

        public override void Timeout()
        {
            Skip();
        }

        public override void LastMinute()
        {
            //TODO: Sound Effect
        }

        public void Work()
        {
            Pomodoro.Instance.Visible = true;
            state = State.Work;
            pomodoroNavigation.SetPanel(PomodoroNavigation.Possibilities.DuringCountdown);
            StartClock(WORK_TIME);
        }

        public void Rest()
        {
            TimeoutScreen timeout = new TimeoutScreen();
            timeout.Show();
            if (clock != null)
                clock.Stop();

            state = State.Rest;
            pomodoroNavigation.SetPanel(PomodoroNavigation.Possibilities.DuringCountdown);
            StartClock(BREAK_TIME);
        }

        public void Skip()
        {
            clock.Stop();

            switch (state)
            {
                case State.Work:
                    Rest();
                    break;
                case State.Rest:
                    Work();
                    break;
                case State.Pause:
                    if (stateBeforeLastPause == State.Work)
                        Rest();
                    else if (stateBeforeLastPause == State.Rest)
                        Work();
                    break;
                default:
                    break;
            }
        }

        public void Pause()
        {
            if (state == State.Rest || state == State.Work)
            {
                clock.Stop();
                stateBeforeLastPause = state;
                state = State.Pause;
            }
            else if (state == State.Pause)
            {
                clock.Start();
                state = stateBeforeLastPause;
            }
        }

        public void Cancel()
        {
            pomodoroNavigation.SetPanel(PomodoroNavigation.Possibilities.DuringIdle);
            clock.Stop();
            Pomodoro.Instance.GetTimeLabel().Text = "00:00";
        }

        enum State
        {
            Work,
            Rest,
            Pause
        }
    }
}

using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;
using System.Media;

namespace Pomodoro
{
    public partial class Pomodoro : Form
    {
        public static Pomodoro Instance;

        private readonly PomodoroTimer pomodoroTimer;
        private readonly PomodoroNavigation pomodoroNavigation;
        private readonly PomodoroNotifyIcon pomodoroNotifyIcon;



        public Label GetTimeLabel()
        {
            return currentTimeLabel;
        }

        public Pomodoro()
        {
            if (Instance == null)
                Instance = this;
            else
                Application.Exit();

            InitializeComponent();

            pomodoroNavigation = new PomodoroNavigation(Navigation);
            pomodoroTimer = new PomodoroTimer(pomodoroNavigation);
            pomodoroNotifyIcon = new PomodoroNotifyIcon(notifyIcon);
        }

        public void ShowToTaskbar()
        {
            if (this.WindowState == FormWindowState.Minimized)
            {
                Hide();
                pomodoroNotifyIcon.Show();
            }
        }

        private void Pomodoro_Resize(object sender, EventArgs e)
        {
            ShowToTaskbar();
        }

        public void Exit(object sender, EventArgs e)
        {
            Application.Exit();
            Close();
        }

        private void Quit_Click(object sender, EventArgs e)
        {
            WindowState = FormWindowState.Minimized;
        }
    }
}

 

Answer 1

Kilka uwaga na szybko:
1. W programach obiektowych unikaj statyczności. Zastanów się czy 'exitFlag' musi być statyczna? 
2. Zamiast:

while (exitFlag == false)

  Lepiej:

while (!exitFlag)

3. Jak coś można zrobić bez singletona to rób to bez singletona. A jak już musisz to użyj właściwości zamiast pola.

4. Raz nazwy prywatnych pól zaczynasz od '_' a raz nie. Wybierz jeden styl i się go trzymaj.

5. Nie ładnie też wygląda jak metody innych klas niż 'Form' operują na kontrolkach. Jeśli chcesz przesyłać tekst do wyświetlenia, prześlij go eventem, który klasa 'Pomodoro' obsłuży. Jeśli chcesz ukryć jakaś kontrolkę to to żądanie też prześlij zdarzeniem wyżej.

6. W metodzie Reset(). Najpierw robisz:

if (clock != null)
    clock.Stop();

 a później uruchamiasz StartClock(BREAK_TIME); który w pierwszej linii robi to samo.

Answer 2

to proszę Cię o chociaż zerknięcie na poniższe klasy

OK, raczej najważniejsza kwestia: po co wszystko jest statycznymi singletonami? To już sugeruje, że design ma problemy.

Druga kwestia, ta klasa abstrakcyjna Timer i dziedzicząca PomodoroTimer też wyglądają dziwnie. Klasa Timer jest abstrakcyjna, wygląda dość abstrakcyjnie... ale znikąd odwołuje się do Pomodoro.Instance, które wygląda strasznie nie na miejscu. Timer, w dodatku abstrakcyjny, powinien bardzo mało wiedzieć o reszcie aplikacji; najlepiej porównać z wbudowanym we framework Timerem ;)

                while (exitFlag == false)
                {
                    Application.DoEvents();
                }

Dobrze rozumiem co tu się dzieje? Ta pętla znikąd przejmuje całą główną pętlę aplikacji? Tego w ogóle nie powinno być :/

Comments

Fakt używam dużo singletonów ale to z przyzwyczajenia z gameDevu ;)
Przeniosłem z Timer część kodu która nie powinna tam być do PomodoroTimer, teraz jest ładnie i schludnie.
Jeśli chodzi o tą pętlę to do tej pory nawet nie wiedziałem co robiła ale bez niej timer wariował, usunąłem ją i kilka rzeczy i teraz StartClock robi to co powinien :D

Dziękuję za porady, jeszcze tylko muszę rozkminić jak odwoływać się do klas nie poprzez singleton...