Matching days of the week

[Srivats1991]

Added a new matcher api to match days of the week in a string. The string can have days starting with a uppercase (starting after a period) or completely lower case. A new test case has been added as well.

Closes #11

[cyucelen]

Hey, i see your message that you are new to Golang. I would like help you with writing some reviews iteratively to make your pull request ready to merge!

1. First of all you don't need to specify size of the string slice. Simply,

// use this notation
daysOfWeek := []string{"monday", "Monday", "tuesday", "Tuesday", "wednesday", "Wednesday", "thursday", "Thursday", "friday", "Friday", "saturday", "Saturday", "sunday", "Sunday"}

// or this notation if you want an array
daysOfWeek := [14]string{"monday", "Monday", "tuesday", "Tuesday", "wednesday", "Wednesday", "thursday", "Thursday", "friday", "Friday", "saturday", "Saturday", "sunday", "Sunday"}

// instead of this
var days [14]string = [14]string {"monday", "Monday", "tuesday", "Tuesday", "wednesday", "Wednesday", "thursday", "Thursday", "friday", "Friday", "saturday", "Saturday", "sunday", "Sunday"}

Also since these are constant values to use, we don't need to allocate them all over again when this matcher called. So we can define it outside function as const, but in Golang only numbers, strings or booleans can be defined as constants (because they can be evaluated at compile-time, slices can not).

var daysOfWeek = [14]string{
    "monday", "tuesday", "wednesday", "thursday", "friday", "saturday", "sunday",
    "Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday", "Sunday",
}
// MatchDaysOfWeek returns a MatcherFunc that matches days of the week in given string
func MatchDaysOfWeek() MatcherFunc {
...

2. Since str is passed by value to the function, there is no need to copy str to newString for preventing mutation of string that is passed to the function.

3. Your matcher implementation wrong due to wrong order of patterns. In order to colorizing patterns in correct order at Mark function, patterns should be at the same order where they been in given string. For example:

str := "Today is Tuesday or tuesday not tUeSday but Tuesday"
actualMatch := MatchDaysofWeek()(str)

actualMatch.Patterns should resemble []string{"Tuesday", "tuesday", "Tuesday"} but in your implementation it looks like []string{"tuesday", "Tuesday"}.

Where is the problem?

• It has 2 elements instead of 3, because your implementation replaces all matches but appends only 1 pattern.

  • It has wrong order because, replacing order depends of the order of daysOfWeek array.

  So you can change your test case to this and try to fix your implementation. I'm waiting for your new commits and questions to help.

Happy Hacktoberfest!

[codecov-io]

Codecov Report

Merging #3 into master will not change coverage. The diff coverage is 100%.

@@          Coverage Diff          @@
##           master     #3   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           3      3           
  Lines          58     76   +18     
=====================================
+ Hits           58     76   +18

Impacted Files	Coverage Δ
matcher.go	100% <100%> (ø)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 984a299...46c8f39. Read the comment docs.

[Srivats1991]

First of all, Thanks a lot !!!! for taking the time to write up in detail. I've made the edits and amended the request by fixing the error in the implementation itself for not taking the orderly matches. Let me know if any additional changes required. Sorry for the trouble. My golang code is still rusty. Hope to get better soon.

[cyucelen]

Hey @Srivats1991 , thanks for your interest again. Lets check out your new commits!

First of all, I wanted you to fix the implementation for make it work with Marker logic correctly and you did a good job as I see, but i want to point out something important. When I look for the tests, saw that you didn't add the test cases for this. So, I want to list some important steps can help you while implementing and committing a feature or fix.

1. Write a new failing test case for the new feature or fix that you are going to implement. 2. Implement your feature or fix just enough for passing the test. Nothing more nothing less. 3. Refactor. Make your code pretty and readable.

You need to keep doing these 3 steps iteratively until you achieve your fully tested implementation. When you regard these steps while writing some code, that makes your code maintainable(that's the point most of the time, because lots of developers have to read, understand and change your code).

So lets review your new commit.

1. Add the new test case and run the test to see it fails.

str := "Today is Tuesday or tuesday not tUesday but Tuesday"
actualMatch := MatchDaysofWeek()(str)
expectedMatch := Match{Template: "Today is %s or %s not tUesday but %s", Patterns: []string{"Tuesday", "tuesday", "Tuesday"}}
assert.Equal(t, expectedMatch, actualMatch)

2. Write the minimal code that can pass the test. You already wrote the implementation that passes this test, so I want to give you some feedback with refactoring.

3. Refactor I want to recommend some variable naming changes firstly, because this improves readability significantly. Open your code aside to track better these part.

• Name your days array as daysOfWeek. Since it is constant, put it out of function body to not allocate it on every call.

  var daysOfWeek = [14]string{
  "monday", "tuesday", "wednesday", "thursday", "friday", "saturday", "sunday",
  "Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday", "Sunday",
  }

• Name your map patternMatchIndexes instead of pattern. Because it stores pattern match indexes right? :) (key is match index, value is pattern)

  patternMatchIndexes := make(map[int]string) // looks like what it intends

• Name your for loop variables meaningfully. They are variables too! We don't want to make them feel bad. For example:

  // if you name your for loop variables this way, every time you read a loop body
  // you are going to go and look for what was this `v` and this reduces readability.
  for _, v := range daysOfWeek

  for _, day := range daysOfWeek // more readable

• Don't be afraid of writing some more characters to make your code crystal clear. For example:

  
  var pat []string // just complete your word

var pattern []string // easy to read

> Programs must be written for people to read, and only incidentally for machines to execute.”—Abelson and Sussman 

So I hope you got my point. Let's make your code more efficient and also more readable with some more refactoring.

In this part you are checking if given string contains the day before you processing. If it is, then you are counting and replacing them etc.

// Your implementation with naming fixes applied for _, day := range daysOfWeek { if strings.Contains(str, day) { count := strings.Count(str, day) for i := 1; i <= count; i++ { matchIndex := strings.Index(str, day) str = strings.Replace(str, day, "%s", 1) patternMatchIndexes[matchIndex] = day } } }

Lets consider this:

for _, day := range daysOfWeek { for strings.Contains(str, day) { matchIndex := strings.Index(str, day) str = strings.Replace(str, day, "%s", 1) patternMatchIndexes[matchIndex] = day } }

Since we are replacing matches with `%s` one by one, we can loop until there is no match and we dont need to call `strings.Count` to calculate how many matches are there for looping that much. By this way we decreased our function's cyclomatic complexity and our function is more readable now.

* Name your `keys` slice that stores the keys of `patternMatchIndexes` map as `matchIndexes`. Also, we know the length of `patternMatchIndexes`, by providing some capacity while creating a slice, we can avoid from some extra memory allocations. If you don't specify any capacity or length, it creates a slice with double capacity and copies all elements every time it need some extra space for appending.

Some examples to creating slice with initial capacity.

matchIndexes := make([]int, 0, len(patternMatchIndexes)) patterns := make([]string, 0, len(patternMatchIndexes))

* Lastly, correct the function name to MatchDays**O**fWeek.

Waiting for your next commit. Have fun! 

[Srivats1991]

Hey, could you please review the changes and move forward with the merge?