You wouldn't. The function doThisAndThat() should never have existed and you're righting that mistake. updateShoppingAddressAndCreditcard() for example should have been a call to UpdateShoppingAddress() followed by a call to UpdateCreditcard(). Those are really two different functions which do two different things and should never have been lumped together. The more underlying idea there is the "single responsibility principle"
@@esven9263no, if they were joined together then that means the business logic requires those 2 things to be updated together. Maybe the UI does that in a bunch of places. Ok, address and CC might be a bad example for this but let's say it's items in the cart and a discount based on the number of items. You can put something in the cart on the product page, edit it in the cart page or destroy the whole cart in the settings and everytime you do that you need to show the correct discount value to the user. You first have to remember to call two functions instead of one everytime(there are multiple elements on each site that interact with the cart and they all need to call those function with slightly different arguments). Second, you have to remember to call the updateCart function first and then call updateDiscount because otherwise it would update the discount on the previous cart state. Third, you can't except a UI developer to care about those implementation details. He has to fight CSS and elements not alighting together. Sorry but you sound like a design pattern monkey, like the ones you can find in Uni teaching an intro to OOP course. So let me break your brain. You either break SRP (creating doThisAndThat function) or you break DRY (not doing that, like you said).
@@esven9263 I'm not entirely sold on the single responsibility principle. I think that separating functions based on their responsibilities is a good thing. But I also think there's a time and a place for a do_this_and_that() function if the things that it does depends on very similar kinds of data and will pretty much always happen together. If you're always calling two functions next to each other, why make it harder to read? Just have one function that serves as a readable list of things that happens. If you then later find that you need to do one function independently of the other, that's then an indication that splitting the function up by responsibility is a good idea. I think however, this should only happen as needed rather than pre-emptively guided by the single responsibility principle.
@@minerscale To me this outer function would describe some standardized process, so you can name it after what prompts this standardized process. Like, Checkout(). But in reality I think it is not very often that this is convenient, since there would be a lot of business logic right in between. I think it is more likely you would composite some set of actions dynamically than have presets for each case.
@@minerscale I think you may not fully understand the single responsibility principle. It doesn't mean you can never create composite functions or objects. It wouldn't be possible to write a program which does much of anything otherwise. However the logic of functions with separate responsibilities should be decoupled until that time. You should maintain a tree structure to your code when possible where functions with specific responsibilities are composited only when shared logic or a shared resource require it. Within the logic of a checkout function for example as @Kredeidi mentioned. The checkout function would no doubt contain a call to updateShoppingAddress() and updateCreditcard() but it's not taking responsibility for how either of those are done. That responsibility has been deligated to those respective functions.
Already at the start, I noticed an issue with your statement about splitting actions like updating an address and updating a credit card. Depending on your database model, how it's connected to the app, and other factors, it could actually be beneficial to update values that reside on the same table. I understand this may seem like bad database design, but in cases where you have legacy systems to work around, you might not have much choice. In that scenario, splitting the action into two would result in two transactions instead of one, and depending on the framework you’re using, it could even grow further since many frameworks operate with transactional windows. What I wanted to convey is that while you are correct in principle, there are situations where splitting actions can lead to undesirable outcomes.
@MisterBloodBunny yup, that is valid point. Almost every good rule in computer science has cases when you should ignore it (and legacy systems can make that happen more often than I think most people would like).
Great video! Giving the thought process of each example really helps There's one doubt I still have: after splitting function A into smaller functions B and C, I still need to call both B and C So whatever class or higher level function that used to call A, still needs all the parameters in order to call B and C In complex systems, it seems like having multiple parameters ends up as unavoidable on the higher functions, or am I missing something?
I think there are two things that you could seen happen: 1) If all the values are getting passed into A, you might be able to combine some of those values into logical structures or class, so while A is getting as much information, there are fewer parameters 2) If A is generating some of the values getting passed to B & C, you can group those values closer to the calls for B & C. So while A is still generating the same data, the sections are (hopefully) more self-contained. So you go from something like: # Generate values for B # Generate values for C B+C(B args + C args) to: # Generate values for B B(args for B) # Generate values for C C(args for C)
4:14 I don't think this is good. `UpdateCountry` usually doesn't make sense unless you also change all other address fields, such as street address. `UpdateAddress` with a bunch of arguments ensures that whoever updates the address doesn't forget to provide all other address fields. Moreover, with such one-line functions it'd be better to just make the fields public, without this unnecessary layer of indirection. The split of `FormattedMailingAddress` is probably good though.
True, if you update a field that represents a larger geographic area then it should logically also update all of the smaller fields (e.g. updating City requires updates to Street Name and Civic Number). However, it is possible that legitimate updates would fail checks on something like this (e.g. moving from London, Ontario, Canada to London, England). It's probably better to just check that the fields are given valid inputs, rather than checking to make sure that a cascade of updates was executed. This class seems to be intended to be "dumb" in the sense that it simply holds on to address information, and pretty-prints it when requested. Making sure that data is collected from the user in a way that makes sense should be the responsibility of the calling code IMO. Also, by having the fields updated through methods rather than public member fields, you can execute validation logic that is specific to the field before the update and throw an error if something is obviously wrong (e.g. trying to update your Country to the value "3").
@DeathBean89 has already made some great points here. The functions are currently just one line, but by having that interface we can later make changes if we want (like adding validation or maybe triggering other systems). On the value of being able to update a single field by itself, I think it's a bit hard to know what makes sense without actually having a bit more context around how this class is used. While we probably don't want an end user to be able to just change their country, if this class is really just responsible for storing the data, we maybe don't want it to have logic to prevent changes if something weird happened (like the London, Canada -> London, England case). I actually had a case a few month back where after I moved, I was able to update most of my address for an account, other than my province. Which was a bit odd :)
yeah, honestly, why wouldn't you write pure math like pure math? If someone is confused then the problem is with the geometry not the code, and no amount of code can avoid that.
For example 1, you are right that there currently isn't any validation, but but having the setter functions now it's possible to add validation (or other changes) later without needing to change the callers. Good point about it being faster in example 2 to avoid the slowness of computing the square root. I'd not really given much thought to that part of the code.
I like these videos a lot man, keep it up!
I had to stop watching halfway through the third example though, but the rest is solid advice.
Thanks!
if you split up a function like "doThisAndThat" into "doThis" and "doThat" how would you call the function that calls both of them?
You wouldn't. The function doThisAndThat() should never have existed and you're righting that mistake. updateShoppingAddressAndCreditcard() for example should have been a call to UpdateShoppingAddress() followed by a call to UpdateCreditcard(). Those are really two different functions which do two different things and should never have been lumped together. The more underlying idea there is the "single responsibility principle"
@@esven9263no, if they were joined together then that means the business logic requires those 2 things to be updated together.
Maybe the UI does that in a bunch of places. Ok, address and CC might be a bad example for this but let's say it's items in the cart and a discount based on the number of items. You can put something in the cart on the product page, edit it in the cart page or destroy the whole cart in the settings and everytime you do that you need to show the correct discount value to the user.
You first have to remember to call two functions instead of one everytime(there are multiple elements on each site that interact with the cart and they all need to call those function with slightly different arguments).
Second, you have to remember to call the updateCart function first and then call updateDiscount because otherwise it would update the discount on the previous cart state.
Third, you can't except a UI developer to care about those implementation details. He has to fight CSS and elements not alighting together.
Sorry but you sound like a design pattern monkey, like the ones you can find in Uni teaching an intro to OOP course. So let me break your brain. You either break SRP (creating doThisAndThat function) or you break DRY (not doing that, like you said).
@@esven9263 I'm not entirely sold on the single responsibility principle. I think that separating functions based on their responsibilities is a good thing. But I also think there's a time and a place for a do_this_and_that() function if the things that it does depends on very similar kinds of data and will pretty much always happen together. If you're always calling two functions next to each other, why make it harder to read? Just have one function that serves as a readable list of things that happens. If you then later find that you need to do one function independently of the other, that's then an indication that splitting the function up by responsibility is a good idea. I think however, this should only happen as needed rather than pre-emptively guided by the single responsibility principle.
@@minerscale To me this outer function would describe some standardized process, so you can name it after what prompts this standardized process. Like, Checkout(). But in reality I think it is not very often that this is convenient, since there would be a lot of business logic right in between.
I think it is more likely you would composite some set of actions dynamically than have presets for each case.
@@minerscale I think you may not fully understand the single responsibility principle. It doesn't mean you can never create composite functions or objects. It wouldn't be possible to write a program which does much of anything otherwise. However the logic of functions with separate responsibilities should be decoupled until that time.
You should maintain a tree structure to your code when possible where functions with specific responsibilities are composited only when shared logic or a shared resource require it. Within the logic of a checkout function for example as @Kredeidi mentioned. The checkout function would no doubt contain a call to updateShoppingAddress() and updateCreditcard() but it's not taking responsibility for how either of those are done. That responsibility has been deligated to those respective functions.
Already at the start, I noticed an issue with your statement about splitting actions like updating an address and updating a credit card. Depending on your database model, how it's connected to the app, and other factors, it could actually be beneficial to update values that reside on the same table. I understand this may seem like bad database design, but in cases where you have legacy systems to work around, you might not have much choice. In that scenario, splitting the action into two would result in two transactions instead of one, and depending on the framework you’re using, it could even grow further since many frameworks operate with transactional windows.
What I wanted to convey is that while you are correct in principle, there are situations where splitting actions can lead to undesirable outcomes.
can't you do batching in that case ? that seems more reasonable than a big function.
@MisterBloodBunny yup, that is valid point. Almost every good rule in computer science has cases when you should ignore it (and legacy systems can make that happen more often than I think most people would like).
Great video! Giving the thought process of each example really helps
There's one doubt I still have: after splitting function A into smaller functions B and C, I still need to call both B and C
So whatever class or higher level function that used to call A, still needs all the parameters in order to call B and C
In complex systems, it seems like having multiple parameters ends up as unavoidable on the higher functions, or am I missing something?
I think it does yes, you'd end up having large objects being sent in as the parameters
I think there are two things that you could seen happen:
1) If all the values are getting passed into A, you might be able to combine some of those values into logical structures or class, so while A is getting as much information, there are fewer parameters
2) If A is generating some of the values getting passed to B & C, you can group those values closer to the calls for B & C. So while A is still generating the same data, the sections are (hopefully) more self-contained.
So you go from something like:
# Generate values for B
# Generate values for C
B+C(B args + C args)
to:
# Generate values for B
B(args for B)
# Generate values for C
C(args for C)
Technically those are parameters.
True. I probably should have been more accurate and referred to it that way. But I think (hope?) most people still understand.
4:14 I don't think this is good. `UpdateCountry` usually doesn't make sense unless you also change all other address fields, such as street address. `UpdateAddress` with a bunch of arguments ensures that whoever updates the address doesn't forget to provide all other address fields.
Moreover, with such one-line functions it'd be better to just make the fields public, without this unnecessary layer of indirection.
The split of `FormattedMailingAddress` is probably good though.
True, if you update a field that represents a larger geographic area then it should logically also update all of the smaller fields (e.g. updating City requires updates to Street Name and Civic Number). However, it is possible that legitimate updates would fail checks on something like this (e.g. moving from London, Ontario, Canada to London, England). It's probably better to just check that the fields are given valid inputs, rather than checking to make sure that a cascade of updates was executed. This class seems to be intended to be "dumb" in the sense that it simply holds on to address information, and pretty-prints it when requested. Making sure that data is collected from the user in a way that makes sense should be the responsibility of the calling code IMO.
Also, by having the fields updated through methods rather than public member fields, you can execute validation logic that is specific to the field before the update and throw an error if something is obviously wrong (e.g. trying to update your Country to the value "3").
@DeathBean89 has already made some great points here. The functions are currently just one line, but by having that interface we can later make changes if we want (like adding validation or maybe triggering other systems).
On the value of being able to update a single field by itself, I think it's a bit hard to know what makes sense without actually having a bit more context around how this class is used. While we probably don't want an end user to be able to just change their country, if this class is really just responsible for storing the data, we maybe don't want it to have logic to prevent changes if something weird happened (like the London, Canada -> London, England case). I actually had a case a few month back where after I moved, I was able to update most of my address for an account, other than my province. Which was a bit odd :)
bad advice.
example 1 there is no need for setters, if they don't do any validation.
example 2 should be 1 line `in_circle = ((x-cx)**2 + (y-cy)**2)
yeah, honestly, why wouldn't you write pure math like pure math? If someone is confused then the problem is with the geometry not the code, and no amount of code can avoid that.
For example 1, you are right that there currently isn't any validation, but but having the setter functions now it's possible to add validation (or other changes) later without needing to change the callers.
Good point about it being faster in example 2 to avoid the slowness of computing the square root. I'd not really given much thought to that part of the code.