1
\$\begingroup\$

I am building a simple meeting scheduler application in c#. I have written following code in c# which is working fine.

void Main()
{
    var sc = new MeetingScheduler();
    sc.Schedule();
}

public class Meeting : IComparable<Meeting>
{
    public DateTime StartTime { get; set; }
    public DateTime EndTime { get; set;}
    public int Duration { get; set;}
    //duration in minutes
    public Meeting(DateTime startTime, int duration)
    {
        this.StartTime = startTime;
        this.Duration = duration;
        this.EndTime = startTime.AddMinutes(duration); ;

    }

    public int CompareTo(Meeting o)
    {
        if (this.EndTime.CompareTo(o.StartTime) < 0)
        {
            return -1;
        }//end time is before the other's start time
        if (this.StartTime.CompareTo(o.EndTime) > 0)
        {
            return 1;
        }////start time is after the other's end time
        return 0;
    }
    public override String ToString()
    {
        return "meeting {" +"from " + StartTime +", minutes=" + Duration +'}';
    }
}

public class MeetingScheduler
{
    private List<Meeting> meetings = new List<Meeting>();
    public Meeting BookRoom(Meeting meeting)
    {
        if (meetings.Count == 0)
        {
            meetings.Add(meeting);
            return null;
        }
        else
        {
            int pos = -Array.BinarySearch(meetings.ToArray(), meeting);
            if (pos > 0)
            {
                meetings.Insert(pos - 1, meeting);
                return null;
            }
            else
            {
                return meetings[-pos];
            }
        }
    }
    public List<Meeting> GetMeetings()
    {
        return meetings;
    }

    public void Schedule()
    {
        MeetingScheduler meetingScheduler = new MeetingScheduler();
        Meeting[] meetingsToBook = new Meeting[]
        {
            new Meeting(new DateTime(2017,09,22,8,30,0), 15),
            new Meeting(new DateTime(2017,09,22,8,44,0), 15),
            new Meeting(new DateTime(2017,09,22,9,10,0), 60),
        };

        foreach (Meeting m in meetingsToBook)
        {
            Meeting oldMeeting = meetingScheduler.BookRoom(m);
            if (oldMeeting != null)
            {
                Console.WriteLine("Could not book room for " + m + " because it collides with " + oldMeeting);
            }
        }

        Console.WriteLine("meetings booked: " + meetingScheduler.GetMeetings().Count());

        foreach (Meeting m in meetingScheduler.GetMeetings())
        {
            Console.WriteLine(m.StartTime + "-> " + m.Duration + " mins");
        }

    }
}

I need some review/comments/suggestion on my implementation

\$\endgroup\$
1
  • \$\begingroup\$ CollidesWith can be replaced with return (thisEndsWhileOtherIsRunning(other)) || thisBeginsWhileOtherIsRunning(other); \$\endgroup\$ Sep 23, 2017 at 11:30

1 Answer 1

2
\$\begingroup\$

Possible Ideas to consider

  1. Returning null values unnecessarily when you should be returning a meeting.
  2. Instantiating an object and then instantiating the same object within itself. You can simply use this See this page here.
  3. You could use public properties rather than GetMeetings() to retrieve the meetings. Using a method is probably more OOP so you’ve done well there.
  4. You can create a method which checks for clashes with other meetings.

Perhaps try working through this refactored code. Hope this helps.

Refactored Code:

internal class Program
{
    private static void Main(string[] args)
    {
        MeetingScheduler sc = new MeetingScheduler();
        sc.Schedule();
    }

    public class Meeting : IComparable<Meeting>
    {
        public DateTime StartTime { get; set; }
        public DateTime EndTime { get; set; }
        public int Duration { get; set; }

        //duration in minutes
        public Meeting(DateTime startTime, int duration)
        {
            this.StartTime = startTime;
            this.Duration = duration;
            this.EndTime = startTime.AddMinutes(duration); ;
        }

        public int CompareTo(Meeting o)
        {
            if (this.EndTime.CompareTo(o.StartTime) < 0)
            {
                return -1;
            }//end time is before the other's start time
            if (this.StartTime.CompareTo(o.EndTime) > 0)
            {
                return 1;
            }////start time is after the other's end time
            return 0;
        }

        public override String ToString()
        {
            return "meeting {" + "from " + StartTime + ", minutes=" + Duration + '}';
        }

        public bool CollidesWith(Meeting other)
        {
            if ((thisEndsWhileOtherIsRunning(other)) || thisBeginsWhileOtherIsRunning(other))
            {
                return true;
            }
            else
            {
                return false;
            }
        }

        private bool thisBeginsWhileOtherIsRunning(Meeting other)
        {
            return this.StartTime >= other.StartTime && this.StartTime <= other.EndTime;
        }

        private bool thisEndsWhileOtherIsRunning(Meeting other)
        {
            return this.EndTime >= other.StartTime && this.EndTime <= other.EndTime;
        }
    }

    public class MeetingScheduler
    {
        private List<Meeting> meetings = new List<Meeting>();

        public void BookRoom(Meeting meeting)
        {
            if (meetings.Any(m => m.CollidesWith(meeting)))
            {
                Console.WriteLine("Could not book room for " + meeting + " because it collides with another meeting");
            }
            else
            {
                meetings.Add(meeting);
                Console.WriteLine("Add meeting: {0} ", meeting);
            }
        }

        public void Schedule()
        {
            Meeting[] meetingsToBook = new Meeting[]{
                                                        new Meeting(new DateTime(2017,09,22,8,30,0), 15),
                                                        new Meeting(new DateTime(2017,09,22,8,44,0), 15),
                                                        new Meeting(new DateTime(2017,09,22,9,10,0), 60),
                                                    };

            foreach (Meeting m in meetingsToBook)
            {
                this.BookRoom(m);
            }

            PrintMeetings();
        }

        private bool NoMeetingsBooked()
        {
            return meetings.Count == 0;
        }

        public List<Meeting> GetMeetings()
        {
            return meetings;
        }

        private void PrintMeetings()
        {
            Console.WriteLine("meetings booked: " + this.GetMeetings().Count());

            foreach (Meeting m in this.GetMeetings())
            {
                Console.WriteLine(m.StartTime + "-> " + m.Duration + " mins");
            }
        }
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ @santoshsingh you found it helpful? \$\endgroup\$
    – BenKoshy
    Sep 23, 2017 at 11:53
  • \$\begingroup\$ Yeah, it's very helpful. Solution is Object oriented and more redable \$\endgroup\$ Sep 23, 2017 at 11:55
  • \$\begingroup\$ I have one small doubt .CompareTo(Meeting o) method is not required because you are not sorting the list \$\endgroup\$ Sep 26, 2017 at 10:01
  • \$\begingroup\$ @santoshsingh that correct - you don't really need the CompareTo method and you also don't need the NoMeetingsBooked() method either. And if you are sorting the list, then you could just as easily use LINQ to do the sorting. \$\endgroup\$
    – BenKoshy
    Sep 26, 2017 at 11:10

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.