Skip to content
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

Code Review #3

Open
anselb opened this issue Aug 4, 2019 · 0 comments
Open

Code Review #3

anselb opened this issue Aug 4, 2019 · 0 comments

Comments

@anselb
Copy link

anselb commented Aug 4, 2019

I think your library is pretty good at the moment. Don't feel like you need to change anything I mention about your code.

Readability and Formatting

  • While I think upperWiggle is a good name and gets the main idea of the function across, I used the name evenCaps to indicate that it is the even indexed characters that get uppercased.
  • I would like to see some comments in your problems file. For example, what is the regex inside of .replace(/\s+/g, '-') doing?
  • Your variable names and indentation are mostly readable besides what I have mentioned.

Organization and Modularity

  • While some of your methods rely on other methods, I think that is okay. Since these methods are bundled together as a library, I don't think someone would need to worry about a method going away, or changing implementation, and breaking the other methods.
  • It seems all of your functions avoid side effects; they seem to all return a new, modified string, rather than modifying the original string.

Standard Library/Conventions

  • Nice usage of the standard library methods. I think it makes the code easier to read than reimplementing it again.
  • While the assignment does call for methods to be created on the string prototype, I think it would be nice to have another file that contains just the functions by themselves. That way, developers have the option of using the extended String methods or importing the independent functions.

Effectiveness of Solution

  • I would say you met all requirements of the assignment. It would have been nice to see oddCaps(), but I think you made the work up by implementing other methods.
  • Because your solution uses a lot of built-ins, I would guess your solutions handle almost all, if not all, edge cases. For example, using .charAt() is great because if that char index does not exist, it will not break.

Testing and Error Handling

  • You're tests seem to cover most, if not all, typical input cases, and some edge cases.
  • I think you could have included some more edge cases in your tests to show that they work, such an empty string, or a string with unusual characters (é or ñ).

Algorithmic Complexity

  • I liked how you called .toUpperCase() and .toLowerCase() on every other character. I believe I just called .toLowerCase() on everything, and then, I called .toUpperCase() on every other character, which is twice as slow.
@noltron000 noltron000 added code quality documentation Improvements or additions to documentation and removed documentation Improvements or additions to documentation labels Oct 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants