Skip to main content
clarify logging/swallowing errors
Source Link
ggorlen
  • 4k
  • 2
  • 18
  • 28

Never swallowGenerally, avoid swallowing errors or logprinting inside of methods that perform logic. Throw errors to the client of your code and let them log the result of the failure, retry the operation with different values, or take whatever other programmatic action they want. Doing I/O is noisy, not useful programmatically and can't be easily suppressed by the client. Pretend you're writing code for the JS standard library--it runs totally silently and you give full control over logging to the client.

Never swallow errors or log inside of methods that perform logic. Throw errors to the client of your code and let them log the result of the failure, retry the operation with different values, or take whatever other programmatic action they want. Doing I/O is noisy, not useful programmatically and can't be easily suppressed by the client. Pretend you're writing code for the JS standard library--it runs totally silently and you give full control over logging to the client.

Generally, avoid swallowing errors or printing inside of methods that perform logic. Throw errors to the client of your code and let them log the result of the failure, retry the operation with different values, or take whatever other programmatic action they want. Doing I/O is noisy, not useful programmatically and can't be easily suppressed by the client. Pretend you're writing code for the JS standard library--it runs totally silently and you give full control over logging to the client.

added 25 characters in body
Source Link
ggorlen
  • 4k
  • 2
  • 18
  • 28

Does append really need to return itsthe value the caller just appended? The caller already has the value and all calls to it ignore the return, so this feels like a sloppy contract.

Does append really need to return its value? The caller already has the value and all calls to it ignore the return, so this feels like a sloppy contract.

Does append really need to return the value the caller just appended? The caller already has the value and all calls to it ignore the return, so this feels like a sloppy contract.

added 45 characters in body
Source Link
ggorlen
  • 4k
  • 2
  • 18
  • 28

Going a step further, remove console.log from all of your code, except during development. Except for extremely rare cases, PRs and production code should never use it--it's noisy, slow, poses a security risk, and pollutes the code. Most serious codebases I've worked on ban it in CI.

aList.pop; is a no-op. Functions need to be called to have an effect: aList.pop(). I suggest learning about and using first-class functions in JS. 

Your tests should be far more comprehensive than the few cursory calls shown here. I haven't run your code, but there are likely other bugs in it. I suggest at least 20 or 30 non-overlapping tests that exercise all aspects of the class. Code slowly and test each method thoroughly, one at a time. Be paranoid and don't trust your code until it's been thoroughly tested.

Make sure functions are honest and transparent about what they do. checkIfSizeIs1 actually checks if the list size is 0, then mutates state without telling the caller.

Link lists arguably should not offer O(n) at/find/insertAt/removeAt operations. I know these are in many standard library implementations, but if you're ever actually using these, you're probably using the wrong data structure. 

If random access is important, use an array which will almost certainly be way more performant than a hand-rolled linked list. Removing these footgun operations solves your traverse and evaluateCondition design problems because you can throw them out entirely. If you do need to keep these, generators, iterators and callback functions will solve your design problems without resorting to ifs and string parameters to control what sort of traversal occurs.

Always use anyall variables you define. Some of the variables in "tests" were never logged, like const at, const contains and const find. Use ESLint to pick up these mistakes.

LinkTaking a step back, link lists are pointless data structures in JS. I'd wager arrays are better for pretty much 99.9% of real use cases. Feel free to provide a counterexample--I'd be genuinely curious to see one used in a real project. I understand this is an educational exercise, though, but worth keeping in mind.

Going a step further, remove console.log from all your code, except during development. Except for extremely rare cases, PRs and production code should never use it--it's noisy, slow, poses a security risk, and pollutes the code. Most serious codebases I've worked on ban it in CI.

aList.pop; is a no-op. Functions need to be called to have an effect: aList.pop(). I suggest learning about and using first-class functions in JS. Your tests should be far more comprehensive than the few cursory calls shown here. I haven't run your code, but there are likely other bugs in it. I suggest at least 20 or 30 non-overlapping tests that exercise all aspects of the class. Code slowly and test each method thoroughly, one at a time. Be paranoid and don't trust your code until it's been thoroughly tested.

Make sure functions are honest and transparent. checkIfSizeIs1 actually checks if the list size is 0, then mutates state without telling the caller.

Link lists arguably should not offer O(n) at/find/insertAt/removeAt operations. I know these are in many standard library implementations, but if you're ever actually using these, you're probably using the wrong data structure. If random access is important, use an array which will almost certainly be way more performant than a hand-rolled linked list. Removing these footgun operations solves your traverse and evaluateCondition design problems because you can throw them out entirely.

Always use any variables you define. Some of the variables in "tests" were never logged, like const at, const contains and const find. Use ESLint to pick up these mistakes.

Link lists are pointless data structures in JS. I'd wager arrays are better for pretty much 99.9% of real use cases. Feel free to provide a counterexample--I'd be genuinely curious to see one used in a real project. I understand this is an educational exercise, though, but worth keeping in mind.

Going a step further, remove console.log from all of your code, except during development. Except for extremely rare cases, PRs and production code should never use it--it's noisy, slow, poses a security risk, and pollutes the code. Most serious codebases I've worked on ban it in CI.

aList.pop; is a no-op. Functions need to be called to have an effect: aList.pop(). 

Your tests should be far more comprehensive than the few cursory calls shown here. I haven't run your code, but there are likely other bugs in it. I suggest at least 20 or 30 non-overlapping tests that exercise all aspects of the class. Code slowly and test each method thoroughly, one at a time. Be paranoid and don't trust your code until it's been thoroughly tested.

Make sure functions are honest and transparent about what they do. checkIfSizeIs1 actually checks if the list size is 0, then mutates state without telling the caller.

Link lists arguably should not offer O(n) at/find/insertAt/removeAt operations. I know these are in many standard library implementations, but if you're ever actually using these, you're probably using the wrong data structure. 

If random access is important, use an array which will almost certainly be way more performant than a hand-rolled linked list. Removing these footgun operations solves your traverse and evaluateCondition design problems because you can throw them out entirely. If you do need to keep these, generators, iterators and callback functions will solve your design problems without resorting to ifs and string parameters to control what sort of traversal occurs.

Always use all variables you define. Some of the variables in "tests" were never logged, like const at, const contains and const find. Use ESLint to pick up these mistakes.

Taking a step back, link lists are pointless data structures in JS. I'd wager arrays are better for pretty much 99.9% of real use cases. Feel free to provide a counterexample--I'd be genuinely curious to see one used in a real project. I understand this is an educational exercise, though, but worth keeping in mind.

added 45 characters in body
Source Link
ggorlen
  • 4k
  • 2
  • 18
  • 28
Loading
deleted 75 characters in body
Source Link
ggorlen
  • 4k
  • 2
  • 18
  • 28
Loading
Source Link
ggorlen
  • 4k
  • 2
  • 18
  • 28
Loading