Add lecodingdev/rwid-git into README.md #3
Conversation
js/functional/palindrome.js
Outdated
| // write your code here | ||
| return true; | ||
| // Remove non-alphabetic characters from the value and convert it to lowercase | ||
| value = value.replace(/[^a-zA-Z]/g, "").toLowerCase(); |
There was a problem hiding this comment.
I guess you want to sanitize the incoming value by using string.replace() API, but the problem is your way to do that is so repetitive. Also, I see the difference sanitize logic between the loop, reverse, and recursive. Please revise this part.
There was a problem hiding this comment.
Add cleanString to avoid repeating the cleaning process for cleanedValue in each function. And fix anitize logic between the loop, reverse, and recursive.
There was a problem hiding this comment.
The logic on cleanString() function is good, just make sure to use a better description at cleanString() function.
js/oop/Factorial.js
Outdated
| const result = | ||
| method === "loop" | ||
| ? new Factorial(n).loop() | ||
| : new Factorial(n).recursive(); |
There was a problem hiding this comment.
This is not how OOP was created on purpose. Could be changed much better than this.
There was a problem hiding this comment.
Change it, but not sure if it correctly
There was a problem hiding this comment.
Create a new method count() at the Factorial class. Then move all of the incoming "method" checking from this main code into that count() method I mentioned before. So now you just do a simple method call count() at the main code.
js/oop/Palindrome.js
Outdated
| let isPalindrome; | ||
|
|
||
| if (method === "reverse") { | ||
| isPalindrome = new Palindrome(word).reverse(); | ||
| } else if (method === "loop") { | ||
| isPalindrome = new Palindrome(word).loop(); | ||
| } else if (method === "recursive") { | ||
| isPalindrome = new Palindrome(word).recursive(); | ||
| } else { | ||
| throw new Error("Method must be reverse, loop, or recursive."); | ||
| } | ||
|
|
||
| const result = isPalindrome | ||
| ? "Yes, this word is a palindrome." | ||
| : "No, this word is not a palindrome."; |
There was a problem hiding this comment.
This is not how OOP was created on purpose. Could be changed much better than this.
There was a problem hiding this comment.
Change it, but not sure if it correctly.
There was a problem hiding this comment.
Refactor this code by doing what I suggest on #3 (comment).
js/oop/Palindrome.js
Outdated
| reverse() { | ||
| const cleanedValue = this.value.toLowerCase().replace(/[^a-z0-9]/g, ""); | ||
|
|
||
| const reversedValue = cleanedValue.split("").reverse().join(""); | ||
|
|
||
| return cleanedValue === reversedValue; | ||
| } | ||
|
|
||
| loop() { | ||
| const cleanedValue = this.value.toLowerCase().replace(/[^a-z0-9]/g, ""); | ||
|
|
||
| const length = cleanedValue.length; | ||
|
|
||
| for (let i = 0; i < length / 2; i++) { | ||
| if (cleanedValue[i] !== cleanedValue[length - 1 - i]) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| recursive() { | ||
| this.value = this.value.replace(/[^a-zA-Z]/g, "").toLowerCase(); | ||
| } |
There was a problem hiding this comment.
Please check my review at #3 (comment).
There was a problem hiding this comment.
change method name reserve to checkPalindromeReverse, loop to checkPalindromeLoop, and recursive to checkPalindromeRecursive.
There was a problem hiding this comment.
Using "Palindrome" phrase on checkPalindromeReverse() and checkPalindromeLoop() is verbose because that class name already tells what it is about. I suggest you to change it into checkUsingReverse() and checkUsingLoop().
README.md
Outdated
| - [lecodingdev/rwid-git](https://github.com/lecodingdev/rwid-git) | ||
|
|
||
| * **and more...** |
There was a problem hiding this comment.
"and more section" is also a unordered list. Please don't remove the "-" mark.
commit message: style: fix removed unordered list at "and more"
There was a problem hiding this comment.
Done add "-" mark to make "and more section" in unordered list.
There was a problem hiding this comment.
Please add this line above your lecoding/dev ... repository to prevent merge conflict.
There was a problem hiding this comment.
Thank you for the PR, you're doing a great job so far. But it seems that the "js/functional/fibonacci.js" and "js/oop/Fibonacci.js" was unfinished yet. Please finish that first before I continue to review your PR. Also please give attention to the comment I wrote on each file. Thanks.
| /** | ||
| * Add cleanString to avoid repeating the cleaning process for cleanedValue in each function. | ||
| * | ||
| * @param {string} value | ||
| */ | ||
| function cleanString(value) { | ||
| return value.toLowerCase().replace(/[^a-z0-9]/g, ""); | ||
| } |
There was a problem hiding this comment.
This function could be placed at "js/helper.js" as it can be used by any function besides palindrome.
|
|
||
| const cleanedValue = cleanString(value); | ||
| const reversedValue = cleanedValue.split("").reverse().join(""); | ||
| return cleanedValue === reversedValue; |
There was a problem hiding this comment.
Put an extra line break at this "return" statement to make it easier to read by the other.
|
|
||
| const cleanedValue = cleanString(value); | ||
| const length = cleanedValue.length; | ||
| for (let index = 0; index < length / 2; index++) { |
There was a problem hiding this comment.
Put an extra line break at this "for" statement to make it easier to read by the other.
| value = parseString(value); | ||
|
|
||
| const cleanedValue = cleanString(value); | ||
| if (index >= Math.floor(cleanedValue.length / 2)) { |
There was a problem hiding this comment.
Put an extra line break at this "if" statement to make it easier to read by the other.
| return 1; | ||
| } | ||
|
|
||
| return this.n * new Factorial(this.n - 1).countUsingRecursive(); |
There was a problem hiding this comment.
No need to re-instance the Factorial class. Just call the countUsingRecursive() method using "this" statement.
| cleanString() { | ||
| this.value = this.value.toLowerCase().replace(/[^a-z0-9]/g, ""); | ||
| } |
There was a problem hiding this comment.
Please refer this function to my comment at #3 (comment). Also I think it just better to return the clean version of the "value" property instead re-write the "value" property when you call this method which means you'll gonna lose the original value of the "value" property.
3 participants
Hello! I just recently forked your repository and want to attach that to the community projects section of your README.md. Looking forward to your response, thank you!