-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Matching days of the week #3
Conversation
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,
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).
2. Since 3. Your matcher implementation wrong due to wrong order of patterns.
actualMatch.Patterns should resemble Where is the problem?
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 Report
@@ Coverage Diff @@
## master #3 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 3 3
Lines 58 76 +18
=====================================
+ Hits 58 76 +18
Continue to review full report at Codecov.
|
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. |
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. 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.
2. Write the minimal code that can pass the test. 3. Refactor
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.
Lets consider this:
Since we are replacing matches with
Some examples to creating slice with initial capacity.
Waiting for your next commit. Have fun! |
Hey, could you please review the changes and move forward with the merge? |
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