I really liked most of what I've seen here, even learned a few things. One thing I really don't like is moving the site struct to a slice of strings. This switches out a useful abstraction and data cohesion for something that is prone to ordering errors and makes it harder to keep the structure in mind.
That was a really great walk through of a code review / pull request! I had a few thoughts: - pulling all of the code back into main seems like it would make it hard to unit test - tests would've been super useful for the code and for me as a viewer - talking about the underlying mechanisms about how tests work and table testing would've removed a lot of the mystery and "ew" that it seems most people associate with testing in go. Thanks for all you do Francesc!
Watching everything get moved into main bothered me. I Feel like the original would have been simpler to add some more edge case handling for, such as detecting if there is any data and raising an error. I did like the refactoring of the goroutines, and as someone relatively new to Go, but not programming, I see where you're coming from, I just didn't know how to fix it in go until now :blush:.
Awesome tutorial! I like this type of videos because it shows your thinking pattern and your workflow. Although I am a newbie in Go, I learned a lot. Thanks for making this video, Looking forward to more such videos.
It's 2020 now and if you're like me and trying to follow along watch until 2:43 and then do the following: git clone github.com/philipithomas/iterscraper git checkout 0b5202d3d79d4e3 # this will set you right at 2:43 in the video. The next git command will create a branch, and lets you save your work. git checkout jforf1_start_point
Thanks! Learned a bunch of useful tricks, especially closing the channel once you finish sending stuff to it so the consumer can use it in a for loop without leaking any goroutine.
Overall, I could see the utility behind most of your changes... bar one. When you removed the log.Fatal on the 429 status of rate limiting, you went beyond just refactoring, and actually changed the behavior of the program in a negative way. Doing log.Fatal caused the program to *stop* when it was rate limited, which surely would be the polite thing to do - if you are querying a website and they say please stop querying us, stopping would be polite. Your change means that the code will *continue* to run every single query, even as each one returns the same, "Please stop hitting me" message from the server, unless there's something about it I'm missing?
But that's one of go's idioms, to return errors from functions. Calling log.Fatal() produces a side effect that, upon initial inspection of the function signature, isn't obvious to the function caller. I do admit, that the way he's done it here isn't the way I would have done it. Instead I would have defined an error constant type, like: _const ErrRateLimited = errors.new("error: rate limited")_ _if resp.StatusCode == http.StatusTooManyRequests {_ _return nil, ErrRateLimited_ _}_ Then, the caller of the function can switch on the error returned and decide what to do next: _results, err := fetch(...)_ _if err == ErrRateLimited {_ _log.Fatal(err)_ _} else if err != nil {_ _log.Print(err)_ _}_
I would like to say that for a bigger real world project keeping things in separate files and respect rate limits are must, as well as many other things. For such a toy project I completely agree with all your choices. Thank you for a great vid.
emitTasks despite being in a goroutine is still synchronous at is serially loops over the url base and adds the id, therefore it's perfectly fine to not have it in a goroutine.
Great video! Thank you so much for posting this. ❤️ One point of confusion for me is at the 26:52 mark you returned nil in the writeSites method. During your GopherCon16 talk you said it was a bad idea to do this. IIRC it was because it could get wrapped by a future method and any comparisons to nil would fail. Could you clear up if this is a bad practice or not?
I think that in this case it's okay, because it's being returned with an error. If the function returns a non-nil error value, then the other returned value should be considered invalid and shouldn't be used further on in the code.
Please do more of this. Like when i open a project on github, i dont know from which file to start. When you check a project, to which files do you look at? Thank you for the videos, please do more projects, much love
Just started to watch these series (finally!!, bit late). Great video from a technical perspective. Just a minor suggestion: coming from being very fond of @mpjme and his “Fun Fun Function” channel myself, I believe it would be maybe good to make it sound more accessible or polite by avoiding phrases like “really simple” or “weird code”. Recognizing that what you are doing might not be that simple for everyone could help to connect better with different types of audiences, overall for developers that have recently started with Go like me (sort of). Awesome video series though, Thanks!!
Actually you already did!, I should have seen more of the series before rushing into making those comments, I really enjoy the videos, thank you again!
Very informative! Just had one question: why would you move the smaller code chunks from tasking.go? Yes, there is an extra file and extra function, but doesn't that make it *more* modular and *more* readable, rather than one huge block of code in the main function block? It would be better IMO to at least move it into its own function if you do not want separate files.
In my opinion, function in Go provides modularity whereas package provides isolation. Keeping the code in tasking.go doesn't seem to provide any extra isolation, rather can be kept in a separate function inside main.go.
Nice . Thanks for the good work. just curious how concurrency forloop goes well with bunch of task channel. when ever i run most of the request goes to one go rountine. i tried having huge delays as well.
don't like replacing site struct with string[]. compiler now cannot check the datatype, you can have some hard to know errors. also, writing everything in main.go is not a good practice. .
Great video In addition what others commented, there is another small issue regarding the performance Both tasks and results are unbuffered channels, capable to hold only a single element. This will block the goroutines if the reader(s) acting slow). I suggest to use some buffering on both, so there will be tasks available for workers and workers can publish the results without latency.
JustForFunc you are right, benchmarking helps. Although it's easy to see, as you increase the number of workers, they will be more likely to finish on the same time and be blocked on the writing to result channel. Small loss, but can be significant in long run.
Really great video, I look forward to seeing more soon! Just a few questions- is it really better to have a function that takes 5 or 6 parameters, rather than use global variables? At what point would it be "too many" parameters? Obviously, you could use a struct if you are regularly passing in the same values, which would probably be cleaner, but it's not always possible. Also, those are some seriously wide tabs you've got there ;)
As some say "If you have a procedure with ten parameters, you probably missed some" I think that if your function receives too many parameters you should reconsider what the function is doing and simplify it also structs help keeping cohesion of the data you're passing
Yep，comparing unbuffered vs buffered chan, having no actual difference(sometimes buffered chan even make worse) single goroutine write to tasks, single goroutine read results, multi fetches which take the most time. Thx Francesc ~
Is there a reason for the preference to use `flag.String` over `flag.StringVar` and dereferencing the pointers everywhere, rather than just moving the global vars to local vars of the `main()` func?
Just for anyone curious and didn't see. It's a feature of the font, combined with "font ligatures". Some IDE's will enable the font without the ligatures unless specifically specified to use them (eg: a check box)
Thanks, this was useful as for a Golang newbie. But as an engineer having experience in other languages, I have to say some of your changes were terrible (mostly in the end, starting with the struct replacement) and went far beyond the refactoring scope.
Nice. This is more than 1 mo wage of the Ethiopian worker sowing that shirt according to Bloomberg: www.bloomberg.com/news/features/2018-03-02/china-is-turning-ethiopia-into-a-giant-fast-fashion-factory We'd better code :-)
I really liked most of what I've seen here, even learned a few things. One thing I really don't like is moving the site struct to a slice of strings. This switches out a useful abstraction and data cohesion for something that is prone to ordering errors and makes it harder to keep the structure in mind.
That was a really great walk through of a code review / pull request!
I had a few thoughts:
- pulling all of the code back into main seems like it would make it hard to unit test
- tests would've been super useful for the code and for me as a viewer
- talking about the underlying mechanisms about how tests work and table testing would've removed a lot of the mystery and "ew" that it seems most people associate with testing in go.
Thanks for all you do Francesc!
Watching everything get moved into main bothered me. I Feel like the original would have been simpler to add some more edge case handling for, such as detecting if there is any data and raising an error.
I did like the refactoring of the goroutines, and as someone relatively new to Go, but not programming, I see where you're coming from, I just didn't know how to fix it in go until now :blush:.
Thanks for a great video, being new to go, I find these kinds of video's a great way to become a better gopher:)
Just great! Fun, simple and useful. Waiting for more.
As a newbie gopher, this is a great watch. I learned a lot. Thanks!
You don’t want to learn anything from this, it’s a terrible, terrible example of extremely poor coding practice.
Awesome tutorial! I like this type of videos because it shows your thinking pattern and your workflow. Although I am a newbie in Go, I learned a lot. Thanks for making this video, Looking forward to more such videos.
It's 2020 now and if you're like me and trying to follow along watch until 2:43 and then do the following:
git clone github.com/philipithomas/iterscraper
git checkout 0b5202d3d79d4e3 # this will set you right at 2:43 in the video. The next git command will create a branch, and lets you save your work.
git checkout jforf1_start_point
Are you aware your playlist is in descending order?
This drove me f**king nuts.
Yes, but you realise that the content should be followed in numerical order. And s o should have the earlier videos on top?
ruclips.net/p/PLYM-T7FMoZiSmZZHlpAsU7KnX6SG9FaBH
Thanks! Learned a bunch of useful tricks, especially closing the channel once you finish sending stuff to it so the consumer can use it in a for loop without leaking any goroutine.
Overall, I could see the utility behind most of your changes... bar one. When you removed the log.Fatal on the 429 status of rate limiting, you went beyond just refactoring, and actually changed the behavior of the program in a negative way. Doing log.Fatal caused the program to *stop* when it was rate limited, which surely would be the polite thing to do - if you are querying a website and they say please stop querying us, stopping would be polite.
Your change means that the code will *continue* to run every single query, even as each one returns the same, "Please stop hitting me" message from the server, unless there's something about it I'm missing?
Came to the comment section to note exactly that. Upon hitting 429 the program should stop or dramatically slow down and retry later.
But that's one of go's idioms, to return errors from functions. Calling log.Fatal() produces a side effect that, upon initial inspection of the function signature, isn't obvious to the function caller.
I do admit, that the way he's done it here isn't the way I would have done it. Instead I would have defined an error constant type, like:
_const ErrRateLimited = errors.new("error: rate limited")_
_if resp.StatusCode == http.StatusTooManyRequests {_
_return nil, ErrRateLimited_
_}_
Then, the caller of the function can switch on the error returned and decide what to do next:
_results, err := fetch(...)_
_if err == ErrRateLimited {_
_log.Fatal(err)_
_} else if err != nil {_
_log.Print(err)_
_}_
I would like to say that for a bigger real world project keeping things in separate files and respect rate limits are must, as well as many other things. For such a toy project I completely agree with all your choices.
Thank you for a great vid.
emitTasks despite being in a goroutine is still synchronous at is serially loops over the url base and adds the id, therefore it's perfectly fine to not have it in a goroutine.
Great video! Thank you so much for posting this. ❤️
One point of confusion for me is at the 26:52 mark you returned nil in the writeSites method. During your GopherCon16 talk you said it was a bad idea to do this. IIRC it was because it could get wrapped by a future method and any comparisons to nil would fail. Could you clear up if this is a bad practice or not?
I think that in this case it's okay, because it's being returned with an error. If the function returns a non-nil error value, then the other returned value should be considered invalid and shouldn't be used further on in the code.
Sorry for using RUclips as stackoverflow, the first command go get does that work today? I am not getting the repo downloaded in my local
First time I look at Go, thanks for the tour!
Please do more of this. Like when i open a project on github, i dont know from which file to start. When you check a project, to which files do you look at? Thank you for the videos, please do more projects, much love
Thanks for the video, I found it really interesting and useful, and a very nice way to understand how you approach a code review ;-)
Just started to watch these series (finally!!, bit late). Great video from a technical perspective. Just a minor suggestion: coming from being very fond of @mpjme and his “Fun Fun Function” channel myself, I believe it would be maybe good to make it sound more accessible or polite by avoiding phrases like “really simple” or “weird code”. Recognizing that what you are doing might not be that simple for everyone could help to connect better with different types of audiences, overall for developers that have recently started with Go like me (sort of). Awesome video series though, Thanks!!
That's totally a good point! I'll try to keep it in mind in the future ❤️
Actually you already did!, I should have seen more of the series before rushing into making those comments, I really enjoy the videos, thank you again!
Great refactoring code, will be wait for next video.Thanks
This is amazing, hopefully someone does more of this
Very high quality upload, thank you for posting :)
Very informative! Just had one question: why would you move the smaller code chunks from tasking.go? Yes, there is an extra file and extra function, but doesn't that make it *more* modular and *more* readable, rather than one huge block of code in the main function block?
It would be better IMO to at least move it into its own function if you do not want separate files.
In my opinion, function in Go provides modularity whereas package provides isolation. Keeping the code in tasking.go doesn't seem to provide any extra isolation, rather can be kept in a separate function inside main.go.
Nice . Thanks for the good work. just curious how concurrency forloop goes well with bunch of task channel. when ever i run most of the request goes to one go rountine. i tried having huge delays as well.
Hey Francesc, really cool video! However, wouldn't it be nice to take functionality out of main.go, as this will make unit testing easier?
don't like replacing site struct with string[]. compiler now cannot check the datatype, you can have some hard to know errors. also, writing everything in main.go is not a good practice. .
Great video
In addition what others commented, there is another small issue regarding the performance
Both tasks and results are unbuffered channels, capable to hold only a single element. This will block the goroutines if the reader(s) acting slow). I suggest to use some buffering on both, so there will be tasks available for workers and workers can publish the results without latency.
+András Laczi thats a big "if" right there. I wouldn't add buffering to the channel unless I prove with profiling and benchmarks this is a problem.
JustForFunc you are right, benchmarking helps. Although it's easy to see, as you increase the number of workers, they will be more likely to finish on the same time and be blocked on the writing to result channel. Small loss, but can be significant in long run.
Why go get instead of 'git clone'?
Nice font, what is it ? love the ligatures
FiraCode with ligatures, yes 😊
Which go vscode plugin do you used ?
Francesc this is so great. Thanks!
Love it! I'd say: keep them coming :) Thanks!
Wow! Insightful content, thanks for sharing!
Really great video, I look forward to seeing more soon!
Just a few questions- is it really better to have a function that takes 5 or 6 parameters, rather than use global variables? At what point would it be "too many" parameters? Obviously, you could use a struct if you are regularly passing in the same values, which would probably be cleaner, but it's not always possible.
Also, those are some seriously wide tabs you've got there ;)
As some say "If you have a procedure with ten parameters, you probably missed some"
I think that if your function receives too many parameters you should reconsider what the function is doing and simplify it
also structs help keeping cohesion of the data you're passing
+JustForFunc I agree. I just noticed that you had a few functions there with a decent amount of parameters.
how to handle concurrency from http request ?
Why not pass a struct as a parameter ?
Why use tasks/results as unbuffered chan?
this will block the multi goroutine until read is done?
Why not? Did you prove that was an actual bottleneck before just assuming it is?
Yep，comparing unbuffered vs buffered chan, having no actual difference(sometimes buffered chan even make worse)
single goroutine write to tasks, single goroutine read results, multi fetches which take the most time.
Thx Francesc ~
Loving the shirt... Where do I get one.
www.ooshirts.com/designapp/sharing/1303261115
Holy Cow, I saw your talk on nil @GopherCon. You literally wore the same shirt... This is so funny XD
I promise I washed it in between :-)
Is there a reason for the preference to use `flag.String` over `flag.StringVar` and dereferencing the pointers everywhere, rather than just moving the global vars to local vars of the `main()` func?
Local variables are easier to track
+JustForFunc also testing is hard with global variables
Read the second part of my question again ;-)
func main() {
var (
a, b, c, d string
e, f int
)
flag.StringVar(&a, `blah`....
+Peter Fern less lines 😊
Christoph Seufert I get that in this case flags will never return a nil pointer, but all that dereferencing without nil checking makes my skin crawl
Excited for more!
Awesome, valuable, helpful. Subscribed!
Why does your != look like a =/= sign? Is that just an IDE thing?
+Gabriel Guzman it's font thing: twitter.com/francesc/status/753638854188339201
Watch episode 10 for more info!
I went straight to the playground to try a ≠. But no, that's not what the compiler expects.
Thanks a lot! Very pretty feature
Just for anyone curious and didn't see. It's a feature of the font, combined with "font ligatures". Some IDE's will enable the font without the ligatures unless specifically specified to use them (eg: a check box)
this was really interesting, thanks for the video
Thanks, this was useful as for a Golang newbie. But as an engineer having experience in other languages, I have to say some of your changes were terrible (mostly in the end, starting with the struct replacement) and went far beyond the refactoring scope.
You are making a great job!!!
good stuff. I hope one day I will be this good
also, damn itunes jumping up and down lol
are you using a virtual server , your entire vscode and terminal is dif than mine.
you git extension is honestly whats dif than mine. What is it called
Nice t-shirt!
all comments of this codebase be like:
var cat Cat // This is a cat
vscode is such a great ide for gophers :D
yes man, where we can get that t-shirt ?? say the secret and don't be evil :)
there you go :-)
www.ooshirts.com/designapp/sharing/1303261115
thanks, ala Barça !!
Nice. This is more than 1 mo wage of the Ethiopian worker sowing that shirt according to Bloomberg:
www.bloomberg.com/news/features/2018-03-02/china-is-turning-ethiopia-into-a-giant-fast-fashion-factory
We'd better code :-)
This is a video about how not to code.
He spends half an hour making a mess of some well factored code, turning it back into 1970’s fortran.
Clubbing everything into main.go , well not a good idea always. :) Thanks for the content :)