 Good afternoon Aloha Ruby How's it going guys Everybody doing well Good lunch Raise your hand if you're in Hawaii right now Not bad right been to worse conference locations. I gotta tell you so it's interesting. I realized earlier This is the first time I've given a talk Twice in a week so on Thursday. I gave this actually this exact same well very similar version of this talk at Magic Ruby Which is in Disney World So I flew directly from Orlando to here. So My life is rough. You can send sympathy cards to my address So just a little bit of administration before we get started My name is Ben Orenstein. I work at Thaplot in Boston and this talk is refactoring from good to great And this is basically about things I've learned in the last year or so that I think improve the quality of my code So I'm just gonna share that with you guys now. I don't want you to think of this as a lecture This isn't me standing here and telling you all these things that are absolute truth. Think of this as pairing I'm now pair programming with all of you So as a result of that if you have questions say something if you disagree say something give you a suggestion say something I love that back and forth during the talks. Don't be shy about interrupting. I love diving into stuff So so go for that Now let's dive right into this I want to show you my first example take a second and read this code Now this is based on some code. I wrote recently. It's a simplified version This is basically a very simple report that takes a collection of orders a start date and end date and returns the total sales of those orders Order has an attribute called place that that we use to check the date range now about a year ago I would have Committed this pushed it up and been done with it Now when I look at this I look at it and I think it's it's about B ish It's about a B B minus. It's decent If I if I was pretty confident that I would never come back to this code and change it or extend it I'd probably just say whatever good enough push it up and let's move on to the next thing But these days I actually there's some stuff I would do to improve it So the first thing I would like to do notice we've got this temp variable right here We're figuring out the orders within range. We're sticking them in a temp and then we're doing something with it I'd actually like to extract this into its own method. I'm gonna pull this into a private method Now I've recorded a macro that does this for me Don't freak out So here's the before Temp is in that method. Here's the after made a private method with the same name and I'm just referencing it from up here So this is something I actually do a lot these days pull out these ten This is this is a factor name for this called extract temp to query and this is something I do a lot Now why do I do this? Why is this worth it? Well, the first thing to notice is we've gone from one method with two lines to two methods with one line each Now I'm not gonna tell you that that's always an improvement, but it usually is I'm starting to think these days that methods longer than a line or a code smell And I should define what code smell is just in case a code smell is something that indicates There may be a problem now that there is a problem that there may be a problem These days I try really hard to get down to methods that are one line and it usually results in really good code Because my methods are focused. So that's one win. The next is that we can reuse this So I think it's reasonably likely given that we have total sales within date range The next thing that the stakeholder asked for is something like average sales within date range Now when the code looks like this you have the logic for selecting those orders within range locked up in one method It's more likely to duplicate that later. Whereas if I have like this, I think it's more likely to me to reuse it So I think that's another win finally The nice thing about this is when the code looks like this for whatever reason because we're programmers because we read code When we see code like this, we read it So the first thing your eye does when you see okay orders in range equals, okay Let me figure out what this is. Let me figure out what this is doing When it looks like this when it's its own method my eye is more likely to see orders within range Okay, that's just private method. I'm just gonna assume that selects all the orders that are within the date range What to do next? I'm more likely to ignore those details and in fact by extracting this method and moving it into a private method I've given you a hint and the hint is this code is not important This is an implementation detail that you don't really need to care about If you care about exactly what's going on in that method you can dive into it But it's not super important to what this report is about So I think the wins are sort of small, but in the aggregate they're worth doing So let's keep moving though. I think we can keep this better. Let's look at this method. We created now orders within range And now within this method We're asking order about things about itself We're saying hey you placed it after the start date and before the end date And then we're using that information to make a decision. Has anybody heard of a concept called tell don't ask Okay, cool. So this is that this is an idea It's not a law, but it's something that can sometimes lead to better code and what tell don't ask is it says It's generally better to send a method send an object a message and have it perform work Rather than ask that object about its internal state and decide what to do on its behalf Not a law, but it's a maximum it tends to lead to better code. This code actually violates tell don't ask You can see I'm trying to select some orders and to do that I ask order about things I ask order things about itself and then make a decision on its behalf So I'd rather actually change this code to look a little bit differently So what if instead it did something like this? So now this sort of looks like an ask, but the logic has moved. I'm actually telling order something I'm saying tell me if you're placed between these two dates as Opposed to hey order. What's your internal status? What are the what kind of attributes do you have and then I'll make a decision based on that? So this is actually closer to following tell don't ask so I have specs for this which I'm going to run right now And that blows up because I use local variables when I meant instance variables. Let's do that again Okay, there's your there's the error. We want undefined method placed at we're calling a method on order I'm effectively doing TDD by writing the code. I wished what wish were there So let's actually add that method to order now, and we actually want not placed at we want place between That's a better name. All right, so let's define that Place between takes a start date and an end date and then We're gonna say is my start is start date after is it before placed at and Is end date? Let's do this over placed at This will be easier to read. It's placed at after start date and Is it before end date? All right So there's our place between method. Let's run the test again. We're back to green So the question was what about a range include and that's a great idea, and I'm gonna get there Yep, we're doing little steps, but this guy's totally right We will end up there eventually but not quite what you said something a little bit slightly better But good question. So, okay, so is this an improvement? Yes, I think it is we've moved this logic over to order We've stopped pulling out Interior pieces of order and fiddling with them and are still just sending messages. This is a good Oh principle to follow don't let the internal details of something like order leak out into code surrounding it That's the that's the core idea behind tell don't ask. So I think we we have a nice win here again I'm happier with where this logic is living, but now we've sort of I See now a new problem a Problem that's made evident now that we have a Couple places where I'm passing start date and end date So notice I pass start date and end date into the initialize of this report And then I'm doing it again down here, and I have start date and date here This is another smell When you have a pair of arguments at least two that you're passing together all the time there's a name for that That's called a data clump, and now a data clump is a great hint that you should extract an object instead to hold those values a Good way to test if you have a data clump by the way Say I these two pieces data have start date and end date if I took one away Would the remaining one make sense? Not really, right? What would it mean to pass it to start creating orders of port with just an end date doesn't really make sense So if these things are always going to live together, there's an implicit reliance on each other, right? It is implicit that I need a start date and an end date Whereas if I made this thing into one object like a date range, it becomes explicit that I need both of those That's another great maxim to think about when you're writing your code Imagine you have to show explain your code to somebody else any line you have to go Okay Well, we're always gonna have a start date and an end date anything that's implicit in the code that you can take out and Somehow make explicit and more obvious. That's generally going to be a really big win So let's do this and we'll start in a spec Here's my spec. It's really simple I have an order within range and then out of range and then this then this date range And then I make sure that the total sales number is correct So let's pull out a date range right now And again, I'm gonna write the code. I wish I had I'm vimming too hard There we go and then here We'll pass in the range Cool so far We're gonna run this it blows up on initialized constant date range as expected because I just referenced a constant That's not real. Let's go make that error go away Okay, now there's gonna be a handful of errors as I run this right because suddenly wrong number of arguments this actually I'm calling date range not new and passing it arguments, but it doesn't expect them So let's just make a quick struct to hold the start date in the end date And now I have roughly the error I want which is wrong number of arguments into orders report. I'm passing two arguments right here It used to expecting three up here So now I need to thread this new date range through and I do this in sort of a compressed series of steps So again, don't freak out. Let's pass in the date range Now there's no such thing as start date end date. It's just date range and Same deal here in that method. We wrote an order It's just a date range and now we need to ask the date range about it started an end date See if we're green again, boom Okay, so Is this a win? Yes, it's a win. I think it is. I think this is worth doing now Bob Martin has said he thinks that most Intermediate object oriented programmers are too reluctant to extract classes You should be fairly aggressive about willing being willing to extract small classes like this. Look at date range It's super simple, right? It's very basic But again, it's made something that used to be implicit in our code Is that better? Better or worse All right It's made something that used to be implicit explicit now Now date range, you know is a pair of values This class doesn't have any behavior on it, but I think this is still worth doing I've created a name for something I've pulled out an explicit name. That's almost always an improvement, but there's another one here Which is we've reduced coupling now. What's coupling coupling is the degree to which two components in a system rely on each other So if component a and component B have zero coupling you can change a as much as you want and in any way You want and you'll never break B Vice versa, right? So that's with no coupling Let's say that a and B are extremely coupled that means it's really hard to make changes to either one without breaking the other Now as you might imagine low coupling is good high coupling is bad Because couple of low coupling makes change easier and that's what's hard about software is responding to change I saw a keynote by Dave Thomas and he said the only thing Worrying about worth worrying about when you look at code is is it easy to change? So low coupling makes things easier to change and let's look at a quick example of some coupling Yeah So there's different types of coupling This is called parameter coupling So notice that notify user of failure passes in an object called failure into another method Within that method within print to console We call two sentence on that parameter Now if I passed nil Into print to console this would blow up right No method error two sentences not defined if I passed us if I passed something else Let's say an integer Into print to console this will blow up right because two sentences not defined on integers Or at least I hope it's not So there's actually coupling between these two methods Notify user of failure has to know What print to console is going to call on its parameter? If I change print to console I could make notify user of failure blow up Because they're coupled through the parameter now parameter coupling isn't bad But less coupling is almost always good So something to notice is methods that take no arguments are superior to methods that take one Because they do not have parameter coupling First time I heard that that blew my mind Also if we keep using induction Methods that take one argument are better than ones that take two And methods that take two arguments are better than the ones that take three Because in each example we're lowering parameter coupling So notice what happened here we just slimmed down Our argument lists from three to two and that's a win So coupling is reduced That's one win we made something explicit that used to be implicit. That's another win But also we now have a great place to hang behavior In object-oriented programming it's a really good idea to group behavior with the data it operates on And now that I look at this Figuring out If a date is between two and two bounds is not actually really good responsibility for order But it's a great responsibility for date range I can imagine wanting to know if other objects Have a date placed between two two end points And we can reuse date range So what I'd like to do now is move this logic into date range So why don't we say What do we want this to look like? So why don't I ask date range? Hey, does this include my place stat? That looks good to me That blows up As expected because this method doesn't exist date And then we'll say is date After the start date And Before the end date Let's see if that works for us and it does So now order just knows how to ask something else If it's place that Is included and I like this because orders should know about place that I'm okay referencing this bit of data Which is on order within order and I'm okay with date range knowing how to find if a date is between two dates Perfect exactly where I want this logic to live Now let's this guy asked about a little refactoring. So let's do that right now There's actually a hand of your way of writing this so I can ask if start date End date the range If it includes date, right? Back to green good But I have a little little thing that I give this talk Uh Somewhere maybe scottish ruby conf and joseph aleem told me this Cover cover or covers let's see Nope cover Yeah So when you ask a range if it includes something ruby is going to instantiate every object within that range And then check if the object you pass into include exists in there If you ask cover, it's just going to figure out it's going to instantiate the two end points And then use some logic to figure out if the thing you passed in is in between this So if we had a date range that was 300 years long This actually could be a lot faster performance wise. Yeah question That's a great point. Um, his point was that my tests aren't really thorough enough I'm not testing the edge conditions in there and that's actually a really good point. I should go back and improve those tests Um, I think you can probably trust me that it works enough for now But yeah, that's a that's a good thing. I should uh, I'll beef up the coverage on that guy because that's a very good point Other questions feature in Good point. Yeah, so the the code smell that motivates this kind of refactoring is called feature nv And feature nv is when one class seems very concerned with the internals of another class What it's asking you a lot of questions about its internal data and working with it That's usually a good hint that you want to move that logic onto the class itself That the code the client code has nv of what that other class knows And so it's asking you a lot of questions and fiddling around with the data Thank you. That's good So a couple little quick things I would do now to refact to finish this up And then we're going to move on to a new example So this is kind of ugly, right? This is pretty janky when I read this Yes, I'm mapping the amount and I'm doing this and I'm summing in this way But if you read this you're just really calculating the total sales, right? So whenever I have a bunch of kind of ugly code that I wouldn't just if I wouldn't use the words Oh, I'm going to map the amount and then inject with zero as the default and add the sum What I would really say if I were describing this to you is then I'll add up the total sales So when you look at your code and it doesn't read like you would say it make it read like you would say it so what I really would say is Something like that where am I type of sorters within range so now Take a look at this top level method The total sales here's the name total sales within date range are the total sales of the orders within range That is how I want my code to look That's like brain dead simple, right? And this is my only I'm only public method I'm extra aggressive about making my public methods read like this because it's really likely that someone that comes into this code Mostly cares about this method These I'm slightly less aggressive about that but people want to know about your public api your public api should be super super clear Your private api should be as well, but it's a little bit less important. It's less likely that people care about those details But even so I'm going to change this guy right here just to be a slightly Quick little hack with ruby one nine I don't need the ampersand symbol plus Oh my goodness So it calls to proc on that for me Oh, that's wonderful. That's cool. Okay, so apparently we don't need to you don't need to call to proc I you can just pass in a symbol argument, which is wonderful Anything else I heard some murmuring So Cory saying I don't need the zero, but he is only partially right Um, if I don't have any orders This will blow up and funny enough I I didn't have the zero originally and then in the middle of a talk jim wyrick is like What happens if you don't have any orders within range? and I was like Map will bring up an empty array and then I'll inject over it and what do I get if I don't have an I guess the return an empty array as opposed to zero, right? If I inject plus on zero Yeah Right if I just have this That's gonna blow up Right I get nil as opposed to zero So jim wyrick beat you on that one, Cory. Sorry You should see my commit message that's like jim wyrick found a bug in my talk. I was really excited Okay, so we've been working hard. So I want to show you as a quick means of a break my most important vim plugin This is the only vim plugin I use obviously Not really But hey, let's move on. Let's do something different There's not a bit of code. I'd like you to look over Now this code represents a job site As in a place. We're doing some construction work Oh, we're cut off the top of the screen a little bit. Let's go up here. There we go Now all job sites have a location because you're always doing work somewhere, but not every job site is going to have a contact Contact could be nil. That's optional. We might not know who the contact is at a particular job site So notice I just described something implicit about this code, right? It's not obvious from looking at this that a contact is Optional, but you can start to see it at these ugly conditionals Look what happens if I want to pull up the contact name I've got a check for the presence of contact. Otherwise I provide a default Likewise with contact phone And if I want to email the contact I have to make sure it exists first before I try to call a method on it So how is this code? Well this code's okay It's not great What's what's the problem? Well, it turns out in a slightly more hidden way. We're still violating tell-don't ask or violating tell-don't ask again So we're actually every time we ask contact something we say hey Do you evaluate to a truthy value? And if you do I know what to do and if not I'll do something else So what we'd rather do is just send contact Contact.name tell me your name and if you do exist return what your name is and if you don't exist tell me no name So what's actually happened here is we've co-opted nil Because when contact doesn't exist at contact will be nil and so I'm actually passing around the nil object the nil singleton As my contact That's not a very good idea and it leads to this ugly conditionals like the one on 12 So how do we prove this? Well, oh and one other bad thing This obscures the point of the code right contact.name really just cares about the contact's name But I have this annoying handling in here. It's making it harder to actually see what this code really does So the way around this is with a pattern reasonably common one called null object pattern So rather than have nil Stand in to mean when there is no contact I'm going to actually create an explicit object to stand in for that case And since this uses the null object pattern, and I believe it's a good idea to actually use pattern names In class names, which will some people want to fight to the death about that one I'm going to call it null contact. So if we don't actually have a contact passed in Assigned a null contact dot new. Let's run our tests Oops got the wrong test going Boom and things blow up. The first error was that there's no null contact So let's make that happen Now we have more errors and now what's happening here is I have tests That don't pass into contact. They just pass in passing nothing And so it's assigning a new null contact here But then null content doesn't respond to things like contact dot name. It's just a brand new class It's got nothing on it. So let's add these things. We're going to add name We're going to add phone and we're going to add deliver personalized email onto our null contact class So here's name When there's no contact the thing I want to return is no name Or no email. It's a deliver personalized email Takes a email body But I'm just going to leave it blank. I'm not going to have it do anything because if there's not a contact just do nothing So if I run this I'm going to find method phone Oh Phone right Back to green and now We're green But what can I do now? I can do whatever programmers favorite thing is which is what? Delete code No one ever fails to know what that is that is so universally the programmers favorite thing Everyone gets that So let's leave some code. Oh, yeah Oh, yeah Oh my god And Where's the green how great is that right? Yeah, thank you That guy's yes that guy likes no contact a lot me too. Okay. So for the cost Of this stupidly simple null contact class I've blown away three conditionals yanked out about 12 lines of code Made these methods more obvious and more direct All for the cost of one little class Also notice we're no longer violating tell don't ask We always tell some sort of contact. Yo, dude, I need your name We never care about whether it's an actual contact or a null contact the client code Doesn't know and it doesn't care. So that's awesome. This is actually a great win this pattern It's simple, but once you understand it you see opportunities to to work with it all the time And it's great now There's a downside now, right What's the downside? Yeah, now I basically have to keep two apis in sync, right If I add new methods to contact I'm going to need to add them to null contact as well That's a bit of a pain in the ass, but it turns out it's generally worth it A lot of code ends up with these big hairy nasty conditionals and null contact just like destroys them like that By the way, this refactoring is called replace conditional with polymorphism Which means rather than doing ifs Just send that same message to multiple different classes. That's polymorphism Right, you just always send a message to varying different types of objects and objects know how to respond to it in different ways So there's a bit of a cost to this So you have to weigh that to see if it's worth it Nothing i'm teaching you today always works. There's no refactoring that is always a great idea You always have to say is this worth it given the components of my system as they are So that's the downside we talked about the good signs There's a lot going on how many people by the way Have code in a rails app that looks like if current user yada yada yada. Where's your hand? Yeah Can you see how this would apply to that? So rather than have current user return a user object or nil and then constantly need to check if current user is nil Have current user return a user or a null user Yeah Yes, it's a great question So the question was Null content is defined in this class Do I leave it in the class or do I pull it out and make it sort of a first class? citizen in my business logic and the answer is it varies Usually with something like a null contact. I will pull this out. I'll put it in app models I'll call it null contact rb. It will live as a top level important class in my app Occasionally I will define classes that only live inside the context of like one other class Right. So like if you are being aggressive about extracting small classes like bob martin says you should be Um, there are times where you'll find that you've written a small class That's really just like a little data container or something very basic That's only used in one class and in those cases. I'll just leave that class Inside the original parent class. I'll even make it private. So I know nothing will ever talk to it Um, it's just for that that top level class By the way, in that case, I wouldn't write explicit tests For that class if it's private to another class, I'm not going to test it I'm not going to test private methods ever actually nothing that's private But if I do promote it into be a top level class Then I'm going to write separate unit tests for this to make sure it works It's a great question Other questions about this Or anything else All right, we got one more example And then a quick wrap up and I got some recommendations and things like that. So Onward and upward Let's go to This guy Take a second and read this code Okay so Let's talk about an idea called depend upon abstractions Now most programmers are aware that's a good idea to depend upon abstractions So most people will use for example active record To have it generate sql for them rather than write sql by hand Unless you're in a horribly ugly situation where sometimes that's the only way out But hopefully you'll get there Also, most people won't just shove bytes into a socket. They'll use a library like nethdtp So most programmers are aware of this idea in general like you want to you'd rather keep pulling up a level of abstraction And you'd rather depend upon those abstractions than particular implementation details For instance, that's why you pull a lot of stuff to be private inside an object You don't want other things outside that code to depend on your internals You want to depend on an abstraction that you're providing The thing that a lot of people don't realize is that abstractions are fractal You can keep going higher and higher and you should be relatively aggressive about creating abstractions within your application One great rule of thumb Is that you want the whole to be simpler than the sum of its parts That's a great rule I took from a book called growing object oriented software guided by tests Which has the longest book title I've ever recommended I'm going to show you a link to that later because it's an awesome book That's a great rule of thumb the sum should be simpler to work with than its parts So if you've got a few models or a few classes in your app Wrap a class around that bad boy make a simple api. It's easier to use Created abstraction that your other things can depend upon rather than the low details within your app One place I see this violated a lot is ideas like this So I have all this code here and this code is concerned with charging people thing for for things brain tree By the way as a payment processor So I pull down the brain tree gem, which is good. It's better than writing direct calls to brain trees api Shoving bytes down a socket. I've extracted a bit or it's using an htp api But my code doesn't know about that. It doesn't know that this is an htp api, which is good We've gone pretty far. This is nice. It's not great Notice that user is concerned With things like how do I find my own brain tree id? And then once I do how do I charge for a subscription and how much should I charge? Or how do I create myself as a customer with inside brain tree? And then refund is concerned with other problems. How do I find the transaction id of the thing I build for? And then how do I refund that? So we're depending upon the brain tree gem as an abstraction But we can go more abstract than that This has some this has some weaknesses right now if I wanted to change Which gem we use to build with brain tree? I suddenly need to open up user That seems like the wrong thing right? Oh, I need to change which payment processing gem I'm using therefore. Let me open up the user class Right, that's a great smell There's this idea called shotgun surgery Where if you need to make an app a change in your app and you need to open up 30 files You're doing surgery with a shotgun right you're blowing stuff code everywhere You're affecting everything you're breaking stuff. It's so much more likely that you're going to break things You're going to do it wrong wouldn't you love to just open up one file and change your payment processing gem there? Yes, you would Isn't it likely that you might change payment processors? Yes, it absolutely is I've done it before Here we have to open up all these classes. So how do we improve upon this? well We do something like I would do something like this I make a new class as you might have guessed. I love classes called payment gateway I've moved that constant inside payment gateway. It's a nice place to hang it And I set up my gateway which by default is brain tree gem And I moved that logic in there And then my client code This stuff only knows about payment gateway And the method names and it just passes itself in Now all that logic Lives inside payment gateway It hasn't leaked into my application If I want to change payment processors There's only one file that needs to change That's the power of depending upon abstraction One file needs to change when I'm going to make a change not shock on surgery That's actually a really big win. It makes it easier to change stuff, which is the thing that really matters Finally notice now if I want to test this guy thoroughly I have to stub methods on brain tree gem Which is a great way to make yourself want to have a bad day. You're going to have a bad time If you stub other people's methods, you're going to have a bad time Brain tree gem His its api is reasonably likely to change you update versions Your tests are stubbed out You don't realize those methods have been changed until you run this thing in production And it blows up and you can't build people for stuff and people are really upset In this version You can stub your own methods if you stub your own methods, you're going to have a good time I'm totally fine stubbing out charge for subscription or create customer because I control them So this thing just got easier to test with the introduction of a new abstraction. That's a great sign That's a positive code smell When you're writing your tests pay attention They're telling you things if it's hard to test something if you have to do something that makes you feel dirty Don't do it listen to those tests and instead Figure out how you can test it and figure out a change you can make to it to test it in an easy and straightforward way I don't always do things like this But when I notice that I've got the concerns of billing spread out through the app I'm more i'm pretty likely to do it I don't like to see things like gem names popping up Or gem class names popping out through all my business logic I'm just concerned that this is a payment gateway. I'm not concerned that I'm using brain tree to do it depend upon abstractions Okay, so we've talked a bit about some different ways of refactoring so When do you refactor? Well, the best time to refactor is when you want to make changes to the code It's actually rare for me to just be like wake up on Monday morning. Be like, you know what? I'm gonna refactor some stuff All right, there's that user class that sucks. I'm gonna refactor it now part of that is because I'm a consultant Right and consulting code is a little different than product code If you're a product company guy versus a consulting guy like at the end of every week I need to point to stuff that I've done and that justifies the value that I've just charged for So there's there's a bit of a There's a bit of a paradigm situation there, which maybe doesn't match yours But I don't just wake up and say I'm gonna refactor stuff But when I go need when I need to change something. I am very likely to go refactor it So Kent Beck had this great tweet, which is if you need to make a change Refactor the code so that the change is easy Note this part might be hard Then make the easy change It's a great way of thinking about it When you get to some code and you think oh man, I got to change this widget to take a foo widget instead Think about how you can make the change that you're about to make really simple and straightforward Refactor it that way and then make your change So don't just refactor willy-nilly Especially if you're a consultant. Okay, so that's a great time to refactor. What are some good things to refactor? What are good candidates for refactoring? Well the first My favorite things to refactor are God objects That's a God object the God object is something a class in your system that everything seems to rely on Or everything seems to interact with and a great rule of thumb is that rails apps almost always have two God objects The first one is user And the second one is whatever that app is about So if it's a to-do list application the other God object is what? To do if it's an e-commerce application the other God object is what? Order exactly Two God objects at least Some apps have more the bigger they are the more likely you've got a handful of God objects So how do you find these God objects? How do you be aware of them? Well? A great way is to walk into your app models directory. I've anonymized this one to protect the guilty Let's get a word count of lines of every object every every model of ours And then let's sort them And you'll see that i'm not totally crazy This is an e-commerce app. How can you tell? Well user is a God object Order is right behind it and this one actually kind of has three merchant is arguably a God object Do you think a 600 line class has only one responsibility? Probably not you've heard of the single responsibility principle, right? Every class should only have one responsibility a single responsibility The first rule of classes is that they should be very small The second rule of classes is that they should be even smaller than that This app is violating that user is huge and because of that it is a pain to work with user So when i'm working with user i refactor it if you want me to change something about user I'm going to make damn sure when I push up those changes user is smaller than when I started I'm extremely aggressive about refactoring responsibilities out of God objects I don't want user to get any bigger. I want to get it smaller same thing with order Same thing with merchant. It's a great way of thinking about these things be aware of your God objects and be completely reluctant to add additional lines of code to them It's going to make your life a lot better Where else is a great time to refactor? High churn files What's churn churn is when you're changing a file a lot I keep having to come back to user and change it and change it and change it and change it Pay attention to that. How do you how do you notice that? Well, maybe you just notice it, but maybe you use a gem like this It's a gem called churn This will look through your git history or svn if you're crazy and It will tell you the files that change the most If a file is changing all the time. It's because you don't really understand it It's a great candidate for refactoring If changed you want to make it easy to change that file in the future So give it a good a good hefty dose of refactoring another great place to do look for refactoring Are files in which you find bugs and you know why because bugs love company If there's a lot a bug on line 10 chances are there's a bug on line 11 And the reason is the code was too complicated for you to understand it That's why the bug is there. It was too hard for you to see the bug So if you've got a file where bugs are showing up all the time Refactoring make it easier to understand give the bugs fewer places to hide So that's when I like to refactor so a couple recommendations before I go First is if you haven't read this book read this book This is about the best like intermediate to advanced to sort of advanced beginner-ish book For general programming knowledge like it's it's just great read this book After you've read that book read this book There's basically nothing I talked about today that isn't in this book. This is the bible You've got to know this book. You should know the names in it. You should know the techniques in it. It is wonderful also these two guys Bob martin over here and martin fallow over here are just the best damn software authors there are No one writes about software and programming better than these guys in my opinion Finally, this is a great book This one is like the least known of those three books The other two a lot of people have read or at least have heard of This book is sort of an unknown. It's awesome. It's a beginner book It's an advanced book. It talks about the ideas in o programming and tdd That I sort of roughly knew implicitly, but very cogently very explicitly It made these ideas make a lot more sense it presented them in ways I'd never thought of and gave me a lot of really good rules of thumb So check out those books Refactor when you got to do it write some good code Thank you