2
\$\begingroup\$

I borrowed the code from this question. I'm trying to implement the delegate in that code. Below is my outcome so far.

What else can be done to improve this code? Any suggestions? I would appreciate suggestions regarding delegate, code reuse, etc.

class Program
{        
    static void Main(string[] args)
    {
        var flights = new List<Flight>
        {
            new Flight {Number = "FL001", DepartureTime = DateTime.Now},
            new Flight {Number = "FL002", DepartureTime = DateTime.Now.AddHours(1)},
            new Flight {Number = "FL003", ArrivalTime = DateTime.Now.AddMinutes(30)},
            new Flight {Number = "FL004", ArrivalTime = DateTime.Now.AddHours(1.30)},
        };

        var tower = new FlightTower(flights);

        // Flight 003 asking for landing
        var flightAskingForLanding= flights.FirstOrDefault(x => x.Number == "FL003");

        if (flightAskingForLanding != null)
        {
            Console.WriteLine(string.Format("Flight {0} \t: Okay to land?", flightAskingForLanding.Number));
            Console.WriteLine(string.Format("Flight Tower \t: {0}", tower.CanLand(flightAskingForLanding)));
        }

        Func<Flight, bool> checkSchedule = x => flightAskingForLanding.ArrivalTime.IsConflictWithFlightTime(x.ArrivalTime) && flightAskingForLanding.Number != x.Number;
        checkSchedule += x => flightAskingForLanding.ArrivalTime.IsConflictWithFlightTime(x.DepartureTime) && flightAskingForLanding.Number != x.Number;

        if (flightAskingForLanding != null)
        {
            Console.WriteLine(string.Format("Flight {0} \t: Okay to land?", flightAskingForLanding.Number));
            Console.WriteLine(string.Format("Flight Tower \t: {0}", tower.CheckRunwayStatus(flightAskingForLanding,
                                                                                            checkSchedule,
                                                                                            x => x.ArrivalTime == flights.Min(f => f.ArrivalTime))));
        }       

        Console.ReadKey();
    }

}


public class FlightTower
{
    private readonly List<Flight> _schedule;

    public FlightTower(List<Flight> schedule)
    {
        _schedule = schedule;
    }

    //this method is to show my initial implementation
    public bool CanTakeOff(Flight flight)
    {
        //todo: add code to check for both arrival time and departure time as in the CanLand method
        var arrivingFlights = _schedule.Where(x => x.ArrivalTime == DateTime.Now);

        if (arrivingFlights.ToList().Count == 0)
        {
            var flightInQueue = _schedule.FirstOrDefault(x => x.DepartureTime == _schedule.Min(c => c.DepartureTime));
            if (flightInQueue != null && flightInQueue.Number == flight.Number)
            {
                return true;
            }
        }
        return false;
    }
    //this method is to show my initial implementation
    public bool CanLand(Flight flight)
    {
        var arrvingFlight = _schedule.Where(x => flight.ArrivalTime.IsConflictWithFlightTime(x.ArrivalTime) && flight.Number != x.Number);
        var depatingFlight = _schedule.Where(x => flight.ArrivalTime.IsConflictWithFlightTime(x.DepartureTime) && flight.Number != x.Number);
        if (arrvingFlight.ToList().Count == 0 && depatingFlight.ToList().Count == 0)
        {                
            var flightInQueue = _schedule.FirstOrDefault(x => x.ArrivalTime == _schedule.Min(c => c.ArrivalTime));
            if (flightInQueue != null && flightInQueue.Number == flight.Number)
                return true;
        }
        return false;
    }

    //this will replace above CanLand and CanTakeOff methods
    public bool CheckRunwayStatus(Flight flight, Func<Flight, bool> checkSchedule, Func<Flight, bool> checkFlightQueue)
    {        
        var flightStatus = _schedule.Where(checkSchedule);
        if (flightStatus.ToList().Count == 0 )
        {
            var flightInQueue = _schedule.FirstOrDefault(checkFlightQueue);
            if (flightInQueue != null && flightInQueue.Number == flight.Number)
                return true;
        }
        return false;
    }
}

public class Flight
{
    public string Number { get; set; }
    public DateTime DepartureTime { get; set; }
    public DateTime ArrivalTime { get; set; }
    public int TotalCrew { get; set; }       
}


public static class DateTimeExtension
{
    public static bool IsConflictWithFlightTime(this DateTime currentFlightTime, DateTime nextFlightTime)
    {
        TimeSpan timeDiff= nextFlightTime - currentFlightTime;
        double totalMinutes = timeDiff.TotalMinutes;
        if (totalMinutes < 0)
            totalMinutes = totalMinutes * -1;

        totalMinutes = Math.Round(totalMinutes);
        if (totalMinutes <= 30.0D)
            return true;
        else
            return false;

    }
}

Here's my implementation of ConductFlightOperations. I'm not sure if this is proper way of doing it.

public void ConductFlightOperations()
    {
        // Flight 003 asking for landing
        var flightAskingForLanding = _schedule.FirstOrDefault(x => x.Number == "FL003");

        if (flightAskingForLanding != null)
        {
            Console.WriteLine(string.Format("Flight {0} \t: Okay to land?", flightAskingForLanding.Number));
            LandingClearance(flightAskingForLanding);
            flightAskingForLanding.Land();
        }          

    }

Here's my implementation of Deconflict. Right now it only check for someFlight.ArrivalTime (which is for Landing flights), what should be proper way to reuse same code to check for someFlight.DepartureTime(for Departing flights)? Should I pass some flag or create new method or something else?

    protected bool Deconflict(Flight someFlight)
    {
        if (_schedule.Any(x => x.Number != someFlight.Number &&  Math.Abs((x.ArrivalTime - someFlight.ArrivalTime).TotalMinutes) < 20D))
            return false;

        if (_schedule.Any(x => x.Number != someFlight.Number && Math.Abs((x.DepartureTime - someFlight.ArrivalTime).TotalMinutes) < 20D))
            return false;

        return true;
    }
\$\endgroup\$
2
  • \$\begingroup\$ Thanks all for their time and efforts. @radarbob most of the pieces are clear now. Please advice for the EDIT in my questions. \$\endgroup\$
    – gmail user
    Nov 10, 2013 at 22:01
  • \$\begingroup\$ @radarbob could you please spend few minutes from your valuable time to give answer on my EDIT part? \$\endgroup\$
    – gmail user
    Nov 12, 2013 at 14:45

2 Answers 2

7
\$\begingroup\$

Be more Object Oriented

Most of the code in Main() should be in the Flight or FlightTower classes. Seems to me that if it's in an appropriate class then its re-usable.

What does a Tower DO?

  • Clear flights to land (i.e. process a landing request)
  • Clear flights to takeoff
  • Deconflict in/out bound traffic

What does a Flight DO?

  • Actually lands
  • Actually takes off

public class FlightTower {
    // I dont know what if anything to return just yet
    public xxx LandingClearance(Flight incomingFlight) {}
    public xxx TakeoffClearance(Flight outgoingFlight) {}
}

public class Flight {
    public xxx Takeoff() {}
    public xxx Land() {}
}

Making up an excuse to use delegates

Flight just "does what it's told" - i.e. whatever method the delegate holds. Takeoff/landing requests gives context as to what kind of instructions we're talking about.

public delegate void FlightInstructions()

public class Tower {
    public void LandingInstructions() {}
    public void HoldingInstructions() {}
    public void TakeoffInstructions() {}
    public void HoldForTakeoffInstructions() {}

    public xxx LandingClearance(Flight incomingFlight) {
        If (Deconflict(incomingFlight)) {
            incomingFlight.Instructions = LandingInstructions;
        else
            incomingFlight.Instructions = HoldingInstructions;
        }
    }

    protected bool Deconflict(Flight someFlight) {}
}

public class Flight {
    public FlightInstructions Instructions;

    public xxx Takeoff() { Instructions(); }
    public xxx Land() { Instructions(); }
}

An abstract Main() is a good thing

public static void Main() {
    FlightTower tower = new FlightTower(BuildFlightSchedule());

   tower.ConductFlightOperations(); // iterates the _schedule - all that junk exposed in the original Main()
}

public static List<Flight> BuildFlightSchedule() {}

Miscellaneous

This:

double totalMinutes = timeDiff.TotalMinutes;
if (totalMinutes < 0)
    totalMinutes = totalMinutes * -1;

Could be this:

double totalMinutes = Math.Abs(timeDiff.TotalMinutes);
\$\endgroup\$
0
2
\$\begingroup\$

In addition to @radarbobs answer, a few more technical points about your code (mainly related to your usage of Linq):

  1. flightAskingForLanding is obtained using FirstOrDefault, yet your code will crash when building checkSchedule as you unconditionally access the variable.
  2. In FlightTower.CanTakeOff(), you do this:

    var arrivingFlights = _schedule.Where(x => x.ArrivalTime == DateTime.Now);
    
    if (arrivingFlights.ToList().Count == 0)
    {
        ...
    }
    
    • It's not necessary to create a list to get the count, the Linq extensions define a Count() extension method on IEnumerable<T>.
    • Furthermore there is an overload for Count() which accepts a predicate so you can shorten this to: if (_schedule.Count(x => x.ArrivalTime == DateTime.Now) == 0)
    • But actually you are not really interested in the count. What you want is "check if there aren't any flight arriving now" so you should use Any() (which can break early if the sequence is not empty): if (!_schedule.Any(x => x.ArrivalTime == DateTime.Now))
  3. This _schedule.FirstOrDefault(x => x.DepartureTime == _schedule.Min(c => c.DepartureTime)) is an O(N2) operation (for every item in schedule find the minimum in schedule and compare). Unfortunately there is no nice "return object where a certain property has the minimum" built into Linq but there are some alternative options (worst case: first find the minimum and store in a local variable and then search the object)
  4. Above two points also apply to CanLand() and CheckRunwayStatus.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.