Do One Thing - How far to take this rule?

Question

Hi,

So there's the "Do One Thing" rule in the book "Clean Code". But how far do we really have to take this.

For example the following statements:

Settings.Default.BaudRate = baudRate;
Settings.Default.COMPort = port;
Settings.Default.DataBits = dataBits;
Settings.Default.Handshake = handshake;
Settings.Default.Parity = parity;
Settings.Default.ReadTimeout = readTimeout;
Settings.Default.WriteTimeout = writeTimeout;
Settings.Default.CommunicationTimeout = communicationTimeout;
Settings.Default.Save();

Ok, sure there's more than one statement here, but it does feel to me like they are just doing one thing. Saving the settings.

I have this in a single function. Would you really take this appart and have a single method for every setting?

When do you stick to this rule and when don't you?

Answer 1

I'd keep them all in a single SaveSettings() function -- if you put them each in their own function, you'd still need to call all those functions from another function.

Answer 2

Looks perfectly valid to me. The obvious method name for that code is SaveSettings, which indicates that the method does exactly one thing. Nothing to worry about.

Answer 3

I'm assuming this rule references when to divide a function into multiple sub-functions.

You have the right idea - saving settings is "one thing" and could be in its own function. Putting each setting in its own function would be overkill.

Another guideline I've heard that might help your understanding of the "one thing" concept: If the function is longer than one or two pages, it can probably be better written by dividing its contents into multiple sub-functions.

Answer 4

The next section of the book, One Level of Abstraction per Function, goes a long way toward answering this question. All of those statements are at the same level of abstraction, so this function is already doing one thing, saving the settings.

Answer 5

Yes, every method should do one thing only. But what is that one thing?

This depends on the level of abstraction your method is in. A method (property) for saving a single setting is a rather low abstraction. The next higher abstraction would then be the proposed SaveSettings method.

Right at the top you have a single method/function main which also does only one thing: The whole program...

Answer 6

On our team we try to follow the one purpose for each function guideline. To help developers out we added a suggestion to our standards that they consider refactoring if a function exceeds 25 lines. So if you had 100 lines of code setting properties, you might consider splitting them up by categories like SaveUserSettings, SaveNetworkSettings, etc..

The end goal is to make the code more readable. If you took your method and split it up into 20 calls each setting a property, I would think it would be more time-consuming to follow and support.

Answer 7

Looking at your code in isolation, it's difficult to say. You could argue that your function does two things - updating the settings then saving the changes. Also, how do you populate the values, are they passed in as arguments to your function (preferable) or does your function get the values itself (doing something else)?

I would follow the Single Responsibility Principle which is summarised as:

There should never be more than one reason for a class to change

and can equally be applied to methods. In your example would there ever be a reason to update the settings, but not save them?

Answer 8

The example code would go better with the question, when to use properties and when to use parameters on a constructor or method to set object state. I know there is a section on this in the .NET framework guidelines, where it discusses the component base pattern (lots of properties and the components internal state might be invalid inbetween the first and last assignment) vs the other pattern (lots of constructor parameters and method parameters and the objects internal state is valid after each line of code executes.

Answer 9

I haven't read that particular book, but as to the concept ...

"One thing" does not mean "one line of code". "One thing" means that everything in the function should be logically related.

I'd quibble with some of the previous posters in saying that "saveSettings" is "one thing". Maybe it's just a carelessness with wording, but I'll take the opportunity to point out the potential trap. In your case, it's more like "saveCommunicationSettings", which I think easily fits the "one thing" definition. If you added "Settings.Default.customerLoyaltyDiscount= ..." to that list, I'd say you're probably on dangerous ground, because you're now mixing communications settings with pricing calculation settings.

In real life, deciding what is reasonable cohesion is not a formula but a judgement call that requires the exercise of intelligence. Should a function that calculates the total amount of an order include sales tax calculation? Arguably that's two things: total price of all items on order AND calculate sales taxes. But you could also argue that it's only one: find the total price of the order, whatever that involves. In practice I often make the decision based on the complexity of the logic. If all that's required to calculate an order total is a simple loop through all the items adding up their prices and then grab a sales tax rate from a table and multiply, I'd probably do it all in one function. If there's more to it than that -- like the system I'm working on these days, calculating pricing involves stock versus custom orders, looking up all sorts of possible discounts, adding in warranties, etc etc -- we really need to break it up.