cronmask-review.md

Review of https://github.com/sarslanhan/cronmask (8bbbec3)

This looks solid, nice job! Here's a code review, mostly stylistics.

---

the method name Matches could be just simply Match. (Somewhat following the regexp package as a convention.)

---

https://github.com/sarslanhan/cronmask/blob/master/cron.go#L24: I would return a public struct CronMask, there's no need for an interface here. There's the advice in Go: "accept interfaces, return structs". It doesn't always apply, but often does, and I think here, too.

---

https://github.com/sarslanhan/cronmask/blob/master/cron.go#L29 The fields of this struct are unnecessarily exported.

---

https://github.com/sarslanhan/cronmask/blob/master/cron.go#L36 I would call it simply field.

---

https://github.com/sarslanhan/cronmask/blob/master/cron.go#L37 The method is unnecessarily exported, should be match(int) bool.

---

https://github.com/sarslanhan/cronmask/blob/master/cron.go#L43 The field implementations (wildcardCronField, constantCronField, rangeCronField, listCronField) don't need to have the match() method on their pointer values, because their state never changes. Actually, it's probably better if they're not used as pointers.

---

https://github.com/sarslanhan/cronmask/blob/master/cron.go#L43 These structs could also just be simply called: wildcard, constant, range and list. Well, not range, but maybe rangeField.

---

https://github.com/sarslanhan/cronmask/blob/master/cron.go#L48 The field Val is unnecessarily exported.

---

https://github.com/sarslanhan/cronmask/blob/master/cron.go#L56 The fields of this struct are unnecessarily exported.

---

https://github.com/sarslanhan/cronmask/blob/master/cron.go#L65 The field Parts is unnecessarily exported.

---

https://github.com/sarslanhan/cronmask/blob/master/cron.go#L109 This logic is repeated 3 times, it could be a function:

func parseValue(fieldIdx int, raw string) (int, error) {
	parsed, err := strconv.Atoi(raw)
	if err != nil {
		return 0, errors.Wrapf(err, "could not parse cron field: %s. invalid list item: %s", fieldStr, p)
	}
	if err := validateRange(fieldIdx, parsed); err != nil {
		return nil, err
	}

	return parsed, nil
}

---

https://github.com/sarslanhan/cronmask/blob/master/cron.go#L166 alternatively could be (or something like that):

var err error
var minute, hour, dayOfMonth, month, week field
fields := []*field{&minute, &hour, &dayOfMonth, &month, &week}
for i, p := range parts {
	if *fields[i], err = parseField(i, p); err != nil {
		return nil, err
	}
}