Ben mentions Martin Fowler's book on refactoring at the end. FYI, his original book is written mainly for Java devs. There is a ruby version he has also co authored with Jay Fields called Refactoring: Ruby Edition.
I have some source from just a week ago to compare with what I've done in the week since I first watched this video... went from stuff that looks OK - would be considered finished by most - to (and I'm not exaggeration here) something I was able to show my 80+ year old grandmother and she could make some sense of it (since it's pretty much just reading English at the level of granularity preached here). Amazing how far I've come after absorbing this talk, thanks very much for this. ^.^
I personally would have loved to have seen (and would likely implement) an extraction of a Subscription class in this example. Definitely a great talk!
tell don't ask, Why not move this code into Order in a method called generate report? The OrderReport object seems to need to unnecessarily query the order a lot. just have the order generate the report for itself?
What's your solution to TDA for when the objects you're querying already have many responsibilities? Sure, following TDA makes classes highly cohesive, but surely delegating even more responsibilities would potentially make important classes violate SRP?
That was a very informative talk. I'm curious to know how you would write the test for the User class after you extracted out PaymentGateway. It seems like the only thing you could test is that the User class is calling the PaymentGateway. If that was tested, would it introduce coupling between the test and the implementation of the class? Or is it inevitable that some tests will know more about the internals of a class (e.g. that it calls a specific method on PaymentGateway)?
Not sure. Maybe! Give it a try and see how it goes. If you struggle, it's probably not a good technique for that scenario :) Seriously though, our CTO (whose opinion I respect greatly on these things) says he often doesn't follow "Tell, Don't Ask" in the view, since views are pretty much ALL asks. So maybe this technique isn't a great fit here.
around 13min, Ben is constructing the DateRange class using Struct with keys. Why go that route vs instantiating a simple class with an attr_reader? Is this some sort of a shortcut?
You read code more than you change it. And a majority of code, does not change. Look at your git annotations - some code is very actively changed, a large majority stabilizes over 1,2,3 years. If we can't predict in advance which code is going to change, why would we prematurely optimize and make it all flexible upfront by extracting to these additional methods? So much indirection. Imagine the amount of mental overhead introduced by this in the name of "it may change in the future". E.g. Bouncing back and forth between method definitions just to see what a five-line method is trying to achieve? Deconstructing perfectly legible, serialized commands into a thousand levels of granularity may make those lines more flexible, but at the cost of code legibility. Break the lines out when it needs to be done, for DRYness, to clarify a non-obvious query, non-obvious logic. But to refactor a call of `time = Time.current` to its own method `def current_time`, is completely unnecessary.
Ben Orenstein you have used open struct in this particular presentation, regarded as one of the slowest in benchmarking. (I presume with time you also not using the same unless I missed something else :) )
Here I'm again, if figured out that I have to share this, then it could've been written like so: class NullObject def method_missing(name) "no #{name}" end end However you'd still have to define deliver_personalised_email (otherwise it'd've failed) or could create something to handle it. Matt Wynne also gave this awesome talk: watch?v=CGN4RFkhH2M Uncle Bob KeyNote: watch?v=WpkDN78P884
Meanwhile in y company, with thousands classes impossible to understand because they are all tangled and code is spread accross thousands 10 lines files... I disagree with this talk. Don't create too many classes and private methods, do that when it is needed and make sure you don't need to understand underlying structures to understand the interfaces.
This guy is preaching from the OOP bible. I like OOP, and he's got a lot of good points, but he takes almost everything to its logical conclusion and that's often not correct. EG, throwing everything into one function is perfectly fine if that behavior is unique. Such a function should only be refactored when it's proven that the behavior is useful in more general cases. Similarly it's often not a good idea to cram behavior onto a class just because you're doing something with that class. You 'd end up with a confusing mess of a class. Good talk but I really hope people don't blindingly drink the OOP koolaid.
"Such a function should only be refactored when it's proven that the behavior is useful in more general cases." It's because your trigger for refactoring is reusability. Seems like his trigger is making implicit code explicit. Both triggers are valid and valuable, but it's important to be aware that there are different things people value when refactoring and that they do it for a different goal.
like he mentioned, none of the refactor he has showed will work on every instances. There is a pros that one need to identify if it is of worthy cause but in general (imo) well designed code is always easier to maintain.
Very strongly disagree. Readability is the most important thing, and throwing everything into one function makes it more difficult to read. He even made this point over and over. This isn't OOP specific either.
nicolas maloeuvre probably because expert rubyists tend to create classes rather than allowing primitives to do the work. One reason for this is encapsulation. I show a similar principle here: JhIYw1Y7pY0
The problem with replacing conditionals with polymorphism is that all of that behavior needs to be put inside the class which often does not adhere to the single responsibility principle. For instance, in the Contact example, you needed to have virtual dispatch on the method Contact::deliver_personalized_email, which means that the Contact class needs to know how to deliver emails. Does your Contact class really need to know how to deliver emails? Does your Contact class need to depend on your EmailService? Would it not have been actually much neater to say something like @email_service.deliver(contact.email, email_body) inside JobSite?
Watching this after 9 years. This is a great talk.
This is my favorite talk to listen to in the background while i program.
Wait. Do you listen to the same talk while you program?
Ben mentions Martin Fowler's book on refactoring at the end. FYI, his original book is written mainly for Java devs. There is a ruby version he has also co authored with Jay Fields called Refactoring: Ruby Edition.
I have some source from just a week ago to compare with what I've done in the week since I first watched this video... went from stuff that looks OK - would be considered finished by most - to (and I'm not exaggeration here) something I was able to show my 80+ year old grandmother and she could make some sense of it (since it's pretty much just reading English at the level of granularity preached here). Amazing how far I've come after absorbing this talk, thanks very much for this. ^.^
I personally would have loved to have seen (and would likely implement) an extraction of a Subscription class in this example.
Definitely a great talk!
This was the best lecture I've heard in a while.
I'm flattered!
Thanks for your great talk, I really learn a lot from it!!!
one of the best presentations I said, very good man!
you are a genius mr aloha ruby man...
I love the use of macro, already using vim from long time, but never used macro to do this extraction, search but could not find this macro.
VIM master, good lord
10:25 even vim gods waste time unfucking vim lol
wow. That's an excellent talk! Kudos.
tell don't ask, Why not move this code into Order in a method called generate report? The OrderReport object seems to need to unnecessarily query the order a lot. just have the order generate the report for itself?
great talk, made it all the way to the end. a nice one for people looking for philosophy AND practice
What's your solution to TDA for when the objects you're querying already have many responsibilities? Sure, following TDA makes classes highly cohesive, but surely delegating even more responsibilities would potentially make important classes violate SRP?
That was a very informative talk.
I'm curious to know how you would write the test for the User class after you extracted out PaymentGateway. It seems like the only thing you could test is that the User class is calling the PaymentGateway. If that was tested, would it introduce coupling between the test and the implementation of the class? Or is it inevitable that some tests will know more about the internals of a class (e.g. that it calls a specific method on PaymentGateway)?
Not sure. Maybe!
Give it a try and see how it goes. If you struggle, it's probably not a good technique for that scenario :)
Seriously though, our CTO (whose opinion I respect greatly on these things) says he often doesn't follow "Tell, Don't Ask" in the view, since views are pretty much ALL asks. So maybe this technique isn't a great fit here.
Thanks!
Thanks for the great talk!
Thank you, bionic sir.
Github repo for this talk is here: github.com/r00k/refactoring-good-to-great
You rock Ben!
Great talk. What vim theme is he using?
around 13min, Ben is constructing the DateRange class using Struct with keys. Why go that route vs instantiating a simple class with an attr_reader? Is this some sort of a shortcut?
both have the same effect. It's just that using Struct gives you the required outcome in much fewer lines of code.
Thanks also! Great!
I see that you make a function called include? but uses range#cover inside it... kinda confusing but it differs person to person xD
You read code more than you change it. And a majority of code, does not change. Look at your git annotations - some code is very actively changed, a large majority stabilizes over 1,2,3 years.
If we can't predict in advance which code is going to change, why would we prematurely optimize and make it all flexible upfront by extracting to these additional methods? So much indirection. Imagine the amount of mental overhead introduced by this in the name of "it may change in the future".
E.g. Bouncing back and forth between method definitions just to see what a five-line method is trying to achieve?
Deconstructing perfectly legible, serialized commands into a thousand levels of granularity may make those lines more flexible, but at the cost of code legibility.
Break the lines out when it needs to be done, for DRYness, to clarify a non-obvious query, non-obvious logic.
But to refactor a call of `time = Time.current` to its own method `def current_time`, is completely unnecessary.
This talk has so much...class :D BA DUM TSS
someone link me to a way i can do that macro method magic he does in ST3. I am like a total noob, but love hotkeys like this.
Ben Orenstein you have used open struct in this particular presentation, regarded as one of the slowest in benchmarking. (I presume with time you also not using the same unless I missed something else :) )
Here I'm again, if figured out that I have to share this, then it could've been written like so:
class NullObject
def method_missing(name)
"no #{name}"
end
end
However you'd still have to define deliver_personalised_email (otherwise it'd've failed)
or could create something to handle it.
Matt Wynne also gave this awesome talk: watch?v=CGN4RFkhH2M
Uncle Bob KeyNote: watch?v=WpkDN78P884
Meanwhile in y company, with thousands classes impossible to understand because they are all tangled and code is spread accross thousands 10 lines files...
I disagree with this talk. Don't create too many classes and private methods, do that when it is needed and make sure you don't need to understand underlying structures to understand the interfaces.
This guy is preaching from the OOP bible. I like OOP, and he's got a lot of good points, but he takes almost everything to its logical conclusion and that's often not correct. EG, throwing everything into one function is perfectly fine if that behavior is unique. Such a function should only be refactored when it's proven that the behavior is useful in more general cases. Similarly it's often not a good idea to cram behavior onto a class just because you're doing something with that class. You 'd end up with a confusing mess of a class.
Good talk but I really hope people don't blindingly drink the OOP koolaid.
I thought it was as cogent and as valid as the point in the video. Refactoring is great but shipped designs change rapidly also.
"Such a function should only be refactored when it's proven that the behavior is useful in more general cases."
It's because your trigger for refactoring is reusability. Seems like his trigger is making implicit code explicit.
Both triggers are valid and valuable, but it's important to be aware that there are different things people value when refactoring and that they do it for a different goal.
like he mentioned, none of the refactor he has showed will work on every instances. There is a pros that one need to identify if it is of worthy cause but in general (imo) well designed code is always easier to maintain.
Very strongly disagree. Readability is the most important thing, and throwing everything into one function makes it more difficult to read. He even made this point over and over. This isn't OOP specific either.
Polymorphism looks a lot like "Steal second".
16:50 why not delete Order class (l.28-32) and replace l.8 by :
@orders.select{ |order|@date_range.include?(order.placed_at)}
nicolas maloeuvre probably because expert rubyists tend to create classes rather than allowing primitives to do the work. One reason for this is encapsulation. I show a similar principle here: JhIYw1Y7pY0
I think it's blackboard.
28:05 - now delegate!
Create OrdersInDateRange class, and remove all date logic from Report. Without this change code isn't even a good, not saying great
The problem with replacing conditionals with polymorphism is that all of that behavior needs to be put inside the class which often does not adhere to the single responsibility principle. For instance, in the Contact example, you needed to have virtual dispatch on the method Contact::deliver_personalized_email, which means that the Contact class needs to know how to deliver emails.
Does your Contact class really need to know how to deliver emails? Does your Contact class need to depend on your EmailService? Would it not have been actually much neater to say something like @email_service.deliver(contact.email, email_body) inside JobSite?