10
\$\begingroup\$

I'm developing a webpage displaying the scheduled pipelines from all projects of a GitLab instance. The scheduled time is expressed using CRON expressions, i.e. five digits as follows:

Wikipedia's description of the five digits of CRON expressions

Because not all future users of my webpage are familiar with CRON expressions, I wrote a script for translating it into a human-readable time expression.

Some important information for understanding the logic behind the code:

  • "7" is not used as an alias for "Sunday" on my GitLab instance.
  • Timezones are not taken into account (...yet). I'm considering all CRON expressions as UTC times for now.
  • Times are expressed in the 24 hours format (so 11:23PM is 23h23)
  • The pipelines that we launch take a lot of time to be fully executed, which is why we don't allow the first digit of the CRON expression to be *.

Some examples of cron-to-string conversion:

0 4 * * * * → "Runs at 04h00 every day"

0 23 * * 0 → "Runs at 23h00 on Sundays"

0 4 1 * * → "Runs at 04h00 on the 1st day of every month"

The code

function convertCronToString(cronExpression) {
  var cron = cronExpression.split(" ");
  var minutes = cron[0];
  var hours = cron[1];
  var dayOfMonth = cron[2];
  var month = cron[3];
  var dayOfWeek = cron[4];

  var cronToString = "Runs at ";

  // Formatting time if composed of zeros
  if (minutes === "0") minutes = "00";
  if (hours === "0") hours = "00";
  // If it's not past noon add a zero before the hour to make it look like "04h00" instead of "4h00"
  else if (hours.length === 1 && hours !== "*") {
    hours = "0" + hours;
  }
  // Our activities do not allow launching pipelines every minute. It won't be processed.
  if (minutes === "*") {
    cronToString =
      "Unreadable cron format. Cron will be displayed in its raw form: " +
      cronExpression;
  }

  cronToString = cronToString + hours + "h" + minutes + " ";

  if (dayOfWeek === "0,6") dayOfWeek = "on weekends";
  else if (dayOfWeek === "1-5") dayOfWeek = "on weekdays";
  else if (dayOfWeek.length === 1) {
    if (dayOfWeek === "*" && dayOfMonth === "*") dayOfWeek = "every day ";
    else if (dayOfWeek === "*" && dayOfMonth !== "*") {
      cronToString = cronToString + "on the " + dayOfMonth;
      if (
        dayOfMonth === "1" ||
        dayOfMonth === "21" ||
        dayOfMonth === "31"
      ) {
        cronToString = cronToString + "st ";
      } else if (dayOfMonth === "2" || dayOfMonth === "22") {
        cronToString = cronToString + "nd ";
      } else if (dayOfMonth === "3" || dayOfMonth === "23") {
        cronToString = cronToString + "rd ";
      } else {
        cronToString = cronToString + "th ";
      }
      cronToString = cronToString + "day of every month";
      return cronToString;
    } else if (dayOfWeek !== "*" && dayOfMonth === "*") {
      switch (parseInt(dayOfWeek)) {
        case 0:
          dayOfWeek = "on Sundays";
          break;
        case 1:
          dayOfWeek = "on Mondays";
          break;
        case 2:
          dayOfWeek = "on Tuesdays";
          break;
        case 3:
          dayOfWeek = "on Wednesdays";
          break;
        case 4:
          dayOfWeek = "on Thursdays";
          break;
        case 5:
          dayOfWeek = "on Fridays";
          break;
        case 6:
          dayOfWeek = "on Saturdays";
          break;
        default:
          cronToString =
            "Unreadable cron format. Cron will be displayed in its raw form: " +
            cronExpression;
          return cronToString;
      }
    }
    cronToString = cronToString + dayOfWeek + " ";
  }

  return cronToString;
}

Question

How could this code be optimized?

Additional information

  • This method is part of a Vue.JS component so I apologize if I forgot to translate some of the Vue-specific into Vanilla Javascript.
  • I have not implemented the code in case the pipeline is supposed to run on a given number of days of the week/month when provided as a suite of numbers separated by commas.
\$\endgroup\$
  • \$\begingroup\$ @422_unprocessable_entity Mmh, that's possible that it's one of the differences between Vue and Javascript. I'll edit to suit the JS syntax. Thanks for spotting this! \$\endgroup\$ – avazula Feb 19 at 10:54
  • 2
    \$\begingroup\$ Since you didn't say you could use , or - in dayOfWeek, I looked up how these are formed. Does your code correctly handle 0,30 0,12 1,8,15,22,29 1-12 *? \$\endgroup\$ – Peilonrayz Feb 19 at 12:29
4
\$\begingroup\$

How could this code be optimized?

I'm not sure if you mean optimized for performance, brevity, readability, maintainability, etc. but I will offer some suggestions below that might suffice in some of those areas.

Overall the function seems a bit long. There is no exact rule about what is too long but there are various conventions developers adhere to (e.g. see answers to What should be the maximum length of a function? on SE SE. You could break the function up into separate functions that handle an individual aspect of parsing the string and/or adding output- for example one to return the output for the time, one for the days, etc. This might also allow for better unit testing if you chose to do that.

I also see two different return statements - one for the case where dayOfWeek.length === 1 combined with dayOfWeek === "*" && dayOfMonth !== "*" and one at the end. It is fine to return early, but should the code also return early when the string calls for execution each minute? - i.e. the following code

if (minutes === "*") {
  cronToString =
    "Unreadable cron format. Cron will be displayed in its raw form: " +
    cronExpression;
}

Unless the browser requirements are such that it won't work (e.g. for IE) you can utilize some features to shorten the code. For example:

var cron = cronExpression.split(" ");
var minutes = cron[0];
var hours = cron[1];
var dayOfMonth = cron[2];
var month = cron[3];
var dayOfWeek = cron[4];

This could be simplified using Array destructuring:

let [minutes, hours, dayOfMonth, month, dayOfWeek] = cronExpression.split(" ");

You could use const instead of let if you don't overwrite those variables (and instead add to the output string).


I see a few places where the string to be returned (i.e. cronToString) is appended to using

cronToString = cronToString + hours + "h" + minutes + " ";

This can be simplified using the shorthand concatenation operator - i.e. +=

cronToString += hours + "h" + minutes + " ";

I see one call to parseInt() - i.e.

parseInt(dayOfWeek)

If you are going to use parseInt(), it is wise to specify the radix using the second parameter - unless you are using a unique number system like hexidecimal, octal, etc. then specify 10 for decimal numbers.

Always specify this parameter to eliminate reader confusion and to guarantee predictable behavior. Different implementations produce different results when a radix is not specified, usually defaulting the value to 10.1

parseInt(dayOfWeek, 10);

While it may not cause a logic error with this code due to the range of numbers checked, it is a good habit to specify that parameter.


Let us talk about that switch statement. Bearing in mind that the code in the question doesn’t exactly use one, I am reminded of this stellar answer where the OP was advised to use a hashmap (since that Code is java). Whenever you have a set of cases in a switch statement that have one line very similar to the majority of all other cases then you might want to consider creating a mapping. This can be done in JavaScript with an array, a plain old JavaScript object (A.K.A. a “POJO”) or a Map. Then just check that the value used in the switch statement matches an index/key of the array/object/map.

Instead of using the switch statement to add the day of the week display, you could put the days of the week into an array (perhaps outside the function to avoid re-assignment whenever the function is called), and check if dayOfWeek matches a key in that array:

const daysOfWeek = ['Sunday', 'Monday', 'Tuesday', 'Wedensday', 'Thursday', 'Friday', 'Saturday'];

Then to use it, reference the key as dayOfWeek:

if (dayOfWeek > -1 && dayOfWeek < 7) {
   dayOfWeek = "on " + daysOfWeek[dayOfWeek];
}
else {
  cronToString =
    "Unreadable cron format. Cron will be displayed in its raw form: " +
    cronExpression;
  return cronToString;
}

You could also consider using date.toLocalDateString() with the weekday option, which would allow offering localized values, but then that would involve constructing a date and calling more functions, which might not be optimal for your use case.

I also looked for a similar way to add the ordinal suffix but don't see anything built-in, though there are various alternatives in the answers to this SO question - some briefer (and more obfuscated) than others...

1https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt#Parameters

\$\endgroup\$
2
\$\begingroup\$

You do a lot of tests. It would be simplier if you implement a few objects containing your different values and you just display these values.

You'd have to do enough objects to cover all cases but you limit the tests to the minimum this way.

Example :

// I won't print every cases but you got the idea
let dayOfWeekWhenNoMonthIsSpecified = {
    '*' : 'every day', '0' : 'on Sundays', '1' : 'on Mondays',
    '0,6' : 'on weekends', '1-5' : 'on weekdays'
};

cronToString = "Runs " + dayOfWeekWhenNoMonthIsSpecified[cron[4]];
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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