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

Implementation of output and error handling #117

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

illicitonion
Copy link
Member

@netlify
Copy link

netlify bot commented Feb 5, 2023

Deploy Preview for cyf-systems ready!

Name Link
🔨 Latest commit 4292b61
🔍 Latest deploy log https://app.netlify.com/sites/cyf-systems/deploys/65c3f03dc533f70007723a86
😎 Deploy Preview https://deploy-preview-117--cyf-systems.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@chettriyuvraj
Copy link
Contributor

  1. Hey! Not sure if this is the right place to comment but very neat and extensible code.

  2. My code feels pretty basic as opposed to this: 2515af3

  3. Any resources in particular on how to 'think' of such structure? Is it design patterns, is it simply reading more code?

@illicitonion
Copy link
Member Author

Hi @chettriyuvraj, I'm glad you're finding the course useful!

You're solution is generally looking really good! (And honestly, quite a lot like the first step of implementating the solution in this PR).

I think most of the differences between yours and this one primarily stemmed from the the desire to be able to write high-quality fine-grained tests for the implementation. I'd encourage you to have a go at writing a complete test suite for your implementation, and see where things are hard to write tests for, and how the techniques in this pull request overcome those problems.

I'll give you one example, and maybe you can find some more:

Your retry-after parsing is inline in a function that both sleeps, and makes an HTTP request - that means that if you wanted to test that you're parsing things properly, your test will be slow (if you want to test how you parse "5", you will sleep for 5 seconds!) and involved (you'll need a server that handles the requests). By extracting out a function parseDelay, it allows us to test that one function in isolation - parsing a string to a time.Duration is a nice pure function so really easy to test.

I think most of the differences between your implementation and this one are similar - extracting a function, or moving something around, so that it's easier to write tests. I'd recommend practicing this, and you'll start thinking this way more. Maybe even try writing some of the tests before the code - anticipating "I'm going to need to parse a retry-after header, I'll write tests for that parsing, then I'll have a handy function I can use for this".

If you want to talk about any details of your code (after you've added the tests), feel free to make a PR in your fork and @ me on it :)

@chettriyuvraj
Copy link
Contributor

chettriyuvraj commented Feb 2, 2024

Hi @illicitonion!

I didn't expect a reply so quickly so I'm going to be very careful about not wasting your time here. I started by trying to segregate the delay-parsing and it turned into a very neat ripple effect of simplifying the code + making the test easy to write - your comment really made sense once that happened.

The mocking of server using the roundtripper was also very easy to understand with a few google search(es) and reading the documentation. I have tagged you on a small implementation detail I did not manage to figure. I intend to complete all the projects so it'd be awesome to have feedback on code from you. I'll make sure I encash those coupons wisely :)

Also, great work with CodeYourFuture!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants