Review - Aufzug-Logik

Hallo,
ich wollte hier meine Klassen zum Bereitstellen einer Auzug-Logik überprüfen lassen,mir ist bewusst das Logic() und ein eigener Thread fehlt.
Als Frage würde ich direkt fragen: Wenn in einem Auftrag(Task), das Event nicht abonniert wurde,wird eine Exception geschmissen. Ich möchte diese eigentlich zum verwender der Klasse durchreiche, muss sie aber eigentlich auch abfangen um das entsprechende Element aus der Task Liste zu entfernen. Wie soll ich das lösen, mit weiteren Events?

Hier der Code:
Elevator.cs

C#-Code:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Threading;
namespace ElevatorCore
{


    public class Elevator
    {

        #region Events
        #endregion
        #region Properties
        private int _currentFloor;
        public int CurrentFloor { get; }
        public int MaxFloor { get; }
        public int MinFloor { get; }
        private List<ElevatorTask> _tasks = new List<ElevatorTask>();

        //Nur Verübergehend, wenn ich genug über Multithreading, wird ein Thread im Konstuktor gestartet...//
        private List<ElevatorTask> Tasks
        {
            get
            {
                return _tasks;
            }
            set
            {
                _tasks = value;
                Logic();
            }
        }
        #endregion
        #region Constructors
        public Elevator(int maxFloor,int minFloor)
        {
            this.MaxFloor = maxFloor;
            this.MinFloor = minFloor;
            this.CurrentFloor = MinFloor;
        }
        #endregion
        #region PublicMethods
        public bool NewCustomer(ElevatorTask task)
        {

            if (task != null && task.ArriveEventIsInitialized)
            {
                _tasks.Add(task);
                return true;
            }
            return false;
        }
        #endregion
        #region PrivateMethods

        private void Logic()
        {
            while(Tasks.Any())
            {

            }
        }
        private bool Move(int value)
        {
            if(FloorIsAvaible(value))
            {
               _currentFloor = (CurrentFloor + value);
                return true;
            }
            return false;
        }
        private bool FloorIsAvaible(int value)
        {
            if (value >= MinFloor && value <= MaxFloor && value != CurrentFloor) return true;
            return false;
        }
        private ElevatorTask GetNearestRequested()
        {
            if (Tasks.Count == 1) return Tasks[0];
            else if (Tasks.Count < 1)
            {
                Tasks.Sort(new ElevatorLevelComparer());
                return Tasks[0];
            }
            else
                throw new InvalidOperationException("No Task found!");

        }
        private ElevatorTask GetNearestInputed()
        {
            if (Tasks.Count == 1) return Tasks[0];
            else if (Tasks.Count < 1)
            {
                Tasks.Sort(new ElevatorInputComparer());
                return Tasks[0];
            }
            else throw new InvalidOperationException("No Task found");
        }
        #endregion
    }
}

ElevatorTask.cs:

C#-Code:

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

namespace ElevatorCore
{
    public struct ElevatorTask : IEquatable<ElevatorTask>
    {
        #region Operators
        public static bool operator == (ElevatorTask task1, ElevatorTask task2)
        {
            if ( ((object)task1 == null ) || (object)task2 == null ) return Object.Equals(task1, task2);
            return task1.Equals(task2);
        }
        public static bool operator != (ElevatorTask task1,ElevatorTask task2)
        {
            return ! task1.Equals(task2);
        }
        public override bool Equals(object obj)
        {
            if (obj == null) return false;
            if (obj is ElevatorTask) return Equals((ElevatorTask)obj);
            return false;
        }
        public bool Equals (ElevatorTask task)
        {
            if ((object)task == null) return false;
            if (this.RequestedFloor != task.RequestedFloor) return false;
            if (this.InputedFloor != task.RequestedFloor) return false;
            return true;
        }
        #endregion
        #region Events
        public event EventHandler<ElevatorArriveEventArgs> ElevatorArrive;
        private void OnElevatorArrive(ElevatorArriveEventArgs args)
        {
            if (ElevatorArrive != null) ElevatorArrive(this, args);
            else throw new InvalidOperationException("Elevator-arrive event not initialized! ");
        }
        #endregion
        #region Methods
        public override int GetHashCode()
        {
           return TaskID.GetHashCode();
        }
        #endregion
        #region Properties
        public int RequestedFloor { get; set; }
        private bool _arrivedAtRequestedFloor;
        public bool ArrivedAtRequestedFloor
        {
            get
            {
                return _arrivedAtRequestedFloor;
            }
            set
            {
                _arrivedAtRequestedFloor = value;
                if (value == true) OnElevatorArrive(new ElevatorArriveEventArgs(RequestedFloor));
            }

        }
        public string TaskID { get; }
        public bool ArriveEventIsInitialized
        {
            get
            {
                return ElevatorArrive != null ? true : false;
            }
        }
        public int? InputedFloor { get; set; }
        #endregion
        #region Constructors
        public ElevatorTask(int requestedFloor,string taskID)
        {
            RequestedFloor = requestedFloor;
            _arrivedAtRequestedFloor = false;
            InputedFloor = null;
            ElevatorArrive = null;
            TaskID = taskID;
        }
        #endregion
    }
}

ElevatorLevelComparer.cs:

C#-Code:

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

namespace ElevatorCore
{
    class ElevatorLevelComparer : IComparer<ElevatorTask>
    {
        public int Compare(ElevatorTask x, ElevatorTask y)
        {
            //null
            if (x == null && y == null) return 0;
            else if (x == null) return -1;
            else if (y == null) return 1;

            //Comparison
            return x.RequestedFloor.CompareTo(y.RequestedFloor);
        }
    }
}

ElevatorInputComparer.cs:

C#-Code:

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

namespace ElevatorCore
{
    class ElevatorInputComparer : IComparer<ElevatorTask>
    {
        public int Compare(ElevatorTask x, ElevatorTask y)
        {
            //null
            if (x == null && y == null) return 0;
            else if (x == null) return -1;
            else if (y == null) return 1;
            //Input null
            if (x.InputedFloor == null && y.InputedFloor == null) return 0;
            else if (x.InputedFloor == null) return -1;
            else if (y.InputedFloor == null) return 1;
            //Comparison
            return x.InputedFloor.Value.CompareTo(y.InputedFloor.Value);
        }
    }
}

LG

Bitte beachte [Hinweis] Wie poste ich richtig? Punkt 6. Formatiere den Code Richtig, dann ist er auch einfacher zu Lesen.

Den Punkt wo die Exception fliegen soll habe ich beim Überfligen jetzt nicht gefunden. Grundlegend kannst du sie mit einem (try) Catch erst mal Abfangen und dann mit einem Throw wider "weiter" schmeißen. Auch kommt es mir seltsam vor das eine Exception geschmissen wir wenn ein Event nicht Abonniert wird?

Für einen Aufzug sieht es jetzt sehr Kompliziert aus,Task sollte für einen Aufzug nicht gebraucht werden. (Wenn überhaupt für ein Gebäude mit mehrern Aufzügen und da kommt an eigendlich auch Ohne aus).

Ich muss Palin zustimmen: viel Code um nichts.

• Die Benennung "ElevatorTask" halte ich für sehr, sehr unglücklich. Ich habe gerade einige Minuten gebraucht, um mitzubekommen, dass es sich nicht um einen Task handelt (wo ich die Ursache der Ausnahme vermutet hätte).

• Vergiss bitte struct. Wenn man es braucht, weiß man es. Hier nicht.

• Operatorüberladungen sind eine clevere Art und Weise, den Code unnötig kompliziert zu machen.

if(value == true) //nein.
return ElevatorArrive != null ? true : false; //doppel-Nein.

[Tipp] Anfängerhinweis == true / == false

• OnElevatorArrive() ist nicht die korrekte Implementierung einer Event-Auslösung

Sind noch etliche weitere Punkte, die mir auffallen, aber um ehrlich zu sein, wimmelt der Code so von Anfängerfehlern, dass ich ein wenig die Motivation verloren habe.

LaTino

Hallo R3turnz,

ich finde Logic() als Namen für eine Methode sehr sehr unglücklich. Das kann alles sein. Niemand schnallt, was die Methode macht. Besser einen geeigneten Namen suchen.

Weiter, (ich pick mal was raus:

private ElevatorTask GetNearestInputed()
        {
            if (Tasks.Count == 1) return Tasks[0];
            else if (Tasks.Count < 1)
            {
                Tasks.Sort(new ElevatorInputComparer());
                return Tasks[0];
            }
            else throw new InvalidOperationException("No Task found");
        }

Uh, stimmt. Einen habe ich noch:

if(!Tasks.Any())
    throw new InvalidOperationException("No Task found");

var minValue = Tasks.Min(p => p.RequestedFloor);
return Tasks.First(task => task.RequestedFloor == minValue);

//und damit kannst du beide Comparer ersetzen, IEquatable kommt sowieso in den Müll

Ja, die Performance ist schlechter, aber ich gehe nicht von mehr als 1000 Etagen aus, und wenn einen das wirklich stört, gibt es immer noch andere, besser skalierende Lösungen.

LaTino

Ich fände es sehr nett, wenn die Kritiker - deren Inhalt sicherlich korrekt ist, ich seh auch genug - insbesondere bei den Nebensätzen auch ein wenig auf das Alter des Hilfesuchenden achten, wenn er es schon preis gibt.
Es wäre schade, wenn er den ein oder anderen Satz falsch aufnimmt und dadurch die Motivation verlieren würde 😉

Ich habe übrigens festgestellt, dass unsere Anfänger sich tatsächlich dadurch motivieren lassen, dass ihre Tests "grün" werden. In dem Sinn ist es vielleicht eine sehr gute Idee, R3turnz, wenn du dich mal in Bezug auf den Komponententest-Projekttyp schlau machst.

• dein Code wird besser (fehlerfreier)
• du kriegst Wissen, das sehr wertvoll ist (eine strukturierte Herangehensweise an das Programmieren ist unbezahlbar)
• du hast direkt Erfolgserlebnisse, auch wenn das Komplettprogramm noch nicht fertig ist

Denke, das wäre eine exzellente Idee.

LaTino