4
\$\begingroup\$

Description:

Calculate the number of concurrently running jobs for any given minute during a day. Each Job is scheduled to run X times a day, with an expected duration of Y. The output is later fed into a scatter graph (not part of this question)

Link to NET fiddler (runnable code): No of concurrent jobs @ NET fiddler

Code:

using System;
using System.Collections.Generic;
                    
public class Program
{
    private static Random rnd = new Random();
    
    public static void Main()
    {
        //Create a List of Jobs, where each job have a List of StartTimes
        var jobs = CreateTestData();
        
        // process jobs and output number of concurrent jobs / minute
        var output = ComputeScatter(jobs);
        
        // dump output
        foreach(var minute in output)
            Console.WriteLine("Minute "  + minute.Key + " ==> " + minute.Value + " concurrent jobs");
    }
    
    private static Dictionary<int, int> ComputeScatter(List<Job> jobs)
    {
        //Calculate StopTime using Job.DurationInSeconds
        var startAndStops = new List<RunData>();
        foreach (var job in jobs) {
            foreach(var jobStartTime in job.StartTimes)
            {
                var endTime = jobStartTime.AddSeconds(job.ExpectedDurationInSeconds);
                startAndStops.Add(new RunData(jobStartTime, endTime));
            }
        }

        //Plott concurrent jobs count against time. Resolution = 1 minute
        var result = new Dictionary<int, int>();
        for (int i = 0; i < 60 * 24; i++)
        {
            foreach (var jobRunRange in startAndStops)
            {
                var timeStart = DateTime.Now.Date.AddMinutes(i);
                var timeEnd = timeStart.AddSeconds(59).AddMilliseconds(999);
                if (DateRangesOverlap(jobRunRange, timeStart, timeEnd))
                {
                    if (result.ContainsKey(i))
                        result[i]++;
                    else
                        result.Add(i, 1);
                }
            }
        }
        return result;
    }
    
    private static bool DateRangesOverlap(RunData jobRun, DateTime otherStart, DateTime otherStop)
    {
        return jobRun.Start <= otherStop && jobRun.Stop >= otherStart;
    }
    
    private static List<Job> CreateTestData()
    {
        var jobNames = new List<string>()
        {
            "FindWaldo", "FalloutFTW", "YouRock", "ThankYou", "LetsDoThis"
        };
        var jobs = new List<Job>();
        var globalStartDate = DateTime.Now.Date;
        foreach (var name in jobNames)
        {
            var job = new Job
            {
                JobName = name,
                ExpectedDurationInSeconds = rnd.Next(10, 6000),
            };
            for (int i = 0; i < 22; i++)
                job.StartTimes.Add(globalStartDate.AddHours(rnd.Next(0,23)).AddMinutes(rnd.Next(0,58)));
            jobs.Add(job);
        }
        return jobs;
    }
}
    public class RunData
    {
        public DateTime Start;
        public DateTime Stop;
        
        public RunData(DateTime start, DateTime stop) { Start = start; Stop = stop; }
    }
    public class Job
    {
        public string JobName { get; set; }
        public int ExpectedDurationInSeconds { get; set; }
        public List<DateTime> StartTimes { get; set; }
        public Job()
        {
            StartTimes = new List<DateTime>();
        }
    }

Critique:

I welcome any and all critique. Performance, readability, simplicity. I'm also interested in if there is another data type I could use instead of DateTime. DateTime seems kind of heavy since I'm not really interested in the dates themselves. Is there a more lightweight option?

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Are you aware of DateOnly and TimeOnly structures? Here is an introduction article: learn.microsoft.com/en-us/dotnet/standard/datetime/… \$\endgroup\$ Apr 24 at 13:38
  • \$\begingroup\$ Tiny suggestion in addition to the thorough answers: mark rnd as readonly. \$\endgroup\$ Apr 24 at 15:57
  • \$\begingroup\$ ... I'm not really interested in the dates themselves. -> How do you guarantee that no job is ever running across midnight? Seems like TimeSpan would be helpful simplifying some code; its calculations takes both date & time into account. \$\endgroup\$
    – radarbob
    Apr 25 at 3:16

2 Answers 2

6
\$\begingroup\$

Welcome to CR. There are lots of minor things to consider with your code. Presented in no particular order:

I would never consider DateTime to be a heavy object. Its internal source object is really a long. I would keep the DateTime in your code.

C#, unlike Java, prefers braces on a separate line. Sometimes you do, and sometimes you don't.

Your Main method has comments that are not needed. A comment should not tell you what you are doing because what you are doing should be evident in clean code. Rather a comment should explain why you are doing. If there is no strong reason to explain why, the comment can be omitted.

When used for internal timings, you should prefer DateTime.UtcNow over DateTime.Now as this would protect you against weird bugs if you are running this when a DST transition occurs. Also, DateTime.Now must first call DateTime.UtcNow, which obviously means UtcNow is faster.

Here at CR, the preference is that every for, foreach, or if statement should use braces. In Main, your last foreach call does not do this. The reason why is not just style. Its for maintenance. In the future, if you add to the code, you may accidentally apply it to the wrong scope. If you always use the braces, then it becomes clear when you add something whether it goes inside the braces or outside.

When composing strings, consider using Interpolated Strings. Let's look again at the last method in Main and apply the many things I talked about:

Original

 // dump output
 foreach(var minute in output)
     Console.WriteLine("Minute "  + minute.Key + " ==> " + minute.Value + " concurrent jobs");

Modified

foreach(var minute in output)
{
    Console.WriteLine($"Minute {minute.Key} ==> {minute.Value} concurrent jobs");
}

Note that I, (1) removed comment, (2) added braces, and (3) use String Interpolation.

I suggest you give deeper consideration to making some properties readonly. For instance, the Job class could easily have the JobName and ExpectedDurationInSeconds as readonly.

Another consideration is to use ExpectedDuration as a TimeSpan object.

Also in Job, the StartTimes property could be defined as an IList<DateTime>. That is, use the interface rather than the class instance.

For the RunData class, I would prefer StartTime and EndTime as property names to make it clear. Besides, Start and Stop sound like action verbs, rather than property names. You could also have a readonly property of Duration such as

public TimeSpan Duration => StopTime - StartTime;

Wrapping up as I need to get back to work, like I said there are a lot of little things to improve.

\$\endgroup\$
4
\$\begingroup\$

Simply inserting your code in my Visual Studio 17.10.0 Preview 5.0 gives me some suggestions as smart tags or through the light bulb/screw driver icon:

  • Make field readonly,
    'new' expression can be simplified:

    private static readonly Random rnd = new();
    
  • CA1854: Prefer the IDictionary.TryGetValue(TKey, out TValue) method,
    CA1864: To avoid double lookup, call 'TryAdd' instead of calling 'Add' with a 'ContainsKey' guard:

    if (result.TryGetValue(i, out int value)) {
        result[i] = value + 1;
    } else {
        result.Add(i, 1);
    }
    

    You can only apply one of the suggestions here.

  • Use explicit type instead of 'var' (for primitive types):

    foreach (string name in jobNames) { ...
    
  • Use primary constructor:

    public class RunData(DateTime start, DateTime stop)
    {
        public DateTime Start = start;
        public DateTime Stop = stop;
    }
    
  • Collection initialization can be simplified (C# 12):

    StartTimes = []; // Instead of 'new List<DateTime>()'
    
  • Job constructor: Warning (active) CS8618 Non-nullable property 'JobName' must contain a non-null value when exiting constructor. Consider declaring the property as nullable. (However, this will only be shown in a nullable context).

  • (In a comment) SPELL: Spelling error - Plott is not a word.

    //Plot concurrent jobs count against time. Resolution = 1 minute
    

Of course this depends on your target framework, C# version, IDE / IDE version and your specific settings.

\$\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.