refactor(cron): make code a bit more expressive

src/cron/c/expression.h

@@ -1,6 +1,7 @@
 #ifndef VIETER_CRON
 #define VIETER_CRON
 
+#include <stdbool.h>
 #include <stdint.h>
 #include <stdlib.h>
 #include <string.h>
@@ -43,6 +44,7 @@ void cron_ce_next(cron_simple_time *out, cron_expression *ce,
 
 void cron_ce_next_from_now(cron_simple_time *out, cron_expression *ce);
 
-enum cron_parse_error cron_ce_parse_expression(cron_expression *out, char *s);
+enum cron_parse_error cron_ce_parse_expression(cron_expression *out,
+											   const char *expression);
 
 #endif

src/cron/c/parse.c

@@ -43,52 +43,53 @@ const uint8_t max_parts = 4;
  */
 cron_parse_error ce_parse_range(uint64_t *out, char *s, uint8_t min,
 								uint8_t max) {
-	size_t slash_index = 0;
-	size_t dash_index = 0;
-	size_t i = 0;
+	size_t slash_index = 0, dash_index = 0;
+	size_t s_index = 0;
+	char cur_char;
+	bool is_valid_character;
 
-	// We first iterate over the string to determine whether it contains a slash
-	// and/or a dash. We know the dash can only be valid if it appears before
-	// the slash.
-	while (s[i] != '\0') {
-		if (s[i] == '/') {
-			// At most one slash is allowed
-			if (i == 0 || slash_index != 0) {
-				return cron_parse_invalid_expression;
-			}
+	while ((cur_char = s[s_index]) != '\0') {
+		is_valid_character = cur_char == '/' || cur_char == '-' ||
+							 cur_char == '*' ||
+							 (cur_char >= '0' && cur_char <= '9');
 
-			slash_index = i;
-
-			s[i] = '\0';
-		} else if (s[i] == '-') {
-			// At most one dash is allowed, and it must be before the slash
-			if (i == 0 || dash_index != 0 || slash_index != 0) {
-				return cron_parse_invalid_expression;
-			}
-
-			dash_index = i;
-
-			s[i] = '\0';
-		} else if (s[i] != '*' && (s[i] < '0' || s[i] > '9')) {
+		if (!is_valid_character) {
 			return cron_parse_invalid_expression;
 		}
 
-		i++;
+		if (cur_char == '/') {
+			if (s_index == 0 || slash_index != 0) {
+				return cron_parse_invalid_expression;
+			}
+
+			slash_index = s_index;
+
+			s[s_index] = '\0';
+		} else if (cur_char == '-') {
+			// At most one dash is allowed, and it must be before the slash
+			if (s_index == 0 || dash_index != 0 || slash_index != 0) {
+				return cron_parse_invalid_expression;
+			}
+
+			dash_index = s_index;
+
+			s[s_index] = '\0';
+		}
+
+		s_index++;
 	}
 
-	// Parse the three possible numbers in the pattern
 	uint8_t start;
 	uint8_t end = max;
 	uint8_t interval = 0;
 
 	if (s[0] == '*') {
-		// A star character is only allowed on its own
-		if (s[1] == '\0' && dash_index == 0) {
-			start = min;
-			interval = 1;
-		} else {
+		if (s[1] != '\0' || dash_index != 0) {
 			return cron_parse_invalid_expression;
 		}
+
+		start = min;
+		interval = 1;
 	} else {
 		SAFE_ATOI(start, s, min, max);
 
@@ -128,6 +129,7 @@ cron_parse_error ce_parse_part(uint64_t *out, char *s, uint8_t min,
 
 	while ((next = strchr(s, ',')) != NULL) {
 		next[0] = '\0';
+
 		res = ce_parse_range(out, s, min, max);
 
 		if (res != cron_parse_ok) {
@@ -147,15 +149,16 @@ cron_parse_error ce_parse_part(uint64_t *out, char *s, uint8_t min,
  * to be dependent on GCC-specific extensions.
  */
 uint8_t uint64_t_popcount(uint64_t n) {
-	uint8_t c = 0;
+	uint8_t set_bits = 0;
 
 	while (n != 0) {
 		// This sets the least significant bit to zero (very cool)
 		n &= n - 1;
-		c++;
+
+		set_bits++;
 	}
 
-	return c;
+	return set_bits;
 }
 
 /*
@@ -169,19 +172,20 @@ uint8_t bf_to_nums(uint8_t **out, uint64_t bf, uint8_t min, uint8_t max) {
 	// the correct value.
 	uint8_t excess_bits = 64 - (max - min + 1);
 	bf = (bf << excess_bits) >> excess_bits;
+
 	uint8_t size = uint64_t_popcount(bf);
 	uint8_t *buf = malloc(size * sizeof(uint8_t));
 
-	uint8_t i = 0, j = 0;
+	uint8_t bit_index = 0, buf_index = 0;
 
-	while (j < size && i <= max - min) {
-		if (((uint64_t)1 << i) & bf) {
+	while (buf_index < size && bit_index <= max - min) {
+		if (((uint64_t)1 << bit_index) & bf) {
 			// Resize buffer if needed
-			buf[j] = min + i;
-			j++;
+			buf[buf_index] = min + bit_index;
+			buf_index++;
 		}
 
-		i++;
+		bit_index++;
 	}
 
 	*out = buf;
@@ -192,9 +196,10 @@ uint8_t bf_to_nums(uint8_t **out, uint64_t bf, uint8_t min, uint8_t max) {
 /*
  * Parse a cron expression string into a cron_expression struct.
  */
-cron_parse_error ce_parse_expression(cron_expression *out, char *s) {
+cron_parse_error ce_parse_expression(cron_expression *out,
+									 const char *expression) {
 	// The parsing functions modify the input string in-place
-	s = strdup(s);
+	char *s = strdup(expression);
 	char *orig_s = s;
 
 	cron_parse_error res = cron_parse_ok;
@@ -203,7 +208,7 @@ cron_parse_error ce_parse_expression(cron_expression *out, char *s) {
 	// Each part is delimited by a NULL byte.
 	uint8_t part_count = 0;
 	char *parts[max_parts];
-	char *next;
+	char *next_space;
 
 	// Skip leading spaces
 	size_t offset = 0;
@@ -214,18 +219,18 @@ cron_parse_error ce_parse_expression(cron_expression *out, char *s) {
 
 	s += offset;
 
-	while (part_count < max_parts && ((next = strchr(s, ' ')) != NULL)) {
-		next[0] = '\0';
-		parts[part_count] = s;
+	while (part_count < max_parts && ((next_space = strchr(s, ' ')) != NULL)) {
+		next_space[0] = '\0';
 
+		parts[part_count] = s;
 		part_count++;
 
 		// Skip multiple spaces
 		offset = 1;
-		while (next[offset] == ' ') {
+		while (next_space[offset] == ' ') {
 			offset++;
 		}
-		s = next + offset;
+		s = next_space + offset;
 	}
 
 	// Each iteration of the loop skips all trailing spaces. This means that, if

src/cron/parse_test.v

@@ -1,59 +1,32 @@
 module cron
 
 fn test_not_allowed() {
+	illegal_expressions := [
+		'4 *-7',
+		'4 *-7/4',
+		'4 7/*',
+		'0 0 30 2',
+		'0 /5',
+		'0  ',
+		'0',
+		'      0',
+		'       0      ',
+		'1 2 3 4~9',
+		'1 1-3-5',
+		'0 5/2-5',
+		'',
+		'1 1/2/3',
+		'*5 8',
+		'x 8',
+	]
+
 	mut res := false
-	parse_expression('4 *-7') or { res = true }
-	assert res
 
-	res = false
-	parse_expression('4 *-7/4') or { res = true }
-	assert res
-
-	res = false
-	parse_expression('4 7/*') or { res = true }
-	assert res
-
-	res = false
-	parse_expression('0 0 30 2') or { res = true }
-	assert res
-
-	res = false
-	parse_expression('0 0 30 2 0') or { res = true }
-	assert res
-
-	res = false
-	parse_expression('0 /5') or { res = true }
-	assert res
-
-	res = false
-	parse_expression('0  ') or { res = true }
-	assert res
-
-	res = false
-	parse_expression('0') or { res = true }
-	assert res
-
-	res = false
-	parse_expression('1 2 3 4~9') or { res = true }
-	assert res
-
-	res = false
-	parse_expression('1 1-3-5') or { res = true }
-	assert res
-
-	res = false
-	parse_expression('0 5/2-5') or { res = true }
-	assert res
-}
-
-fn test_leading_star() {
-	mut x := false
-	parse_expression('*5 8') or { x = true }
-	assert x
-
-	x = false
-	parse_expression('x 8') or { x = true }
-	assert x
+	for exp in illegal_expressions {
+		res = false
+		parse_expression(exp) or { res = true }
+		assert res, "'$exp' should produce an error"
+	}
 }
 
 fn test_auto_extend() ! {