1

Occassinally I have to write methods like this:

string GetReportOutputDirectoryAndMakeSureExist()
{
    string path = Path.Combine ( ... ) //whatever logic 
    if(!Directory.Exists(path)) Directory.Create(path);
    return path;
}

or

string GetAndVerifyExistenceOfReportConfigurationPath()
{
    string path = Path.Combine ( ... ) //whatever logic to find the configuration
    if(!File.Exists(path)) throw new Exception("Report configuration not found");
    return path;
}

or 

Customer GetCustomerAndVerifyActive(int id)
{
    Customer customer = Db.GetCustomerById(id);
    if(!customer.IsActive) throw new Exception("Customer is not active");
    return customer;
}

Is it a good practice? I am told that it is normally not a good idea for a method to do more than one things, or for a method to have side-effects (like creating directory). But if I split, for example the last metod to GetCustomer(id) and VerifyActive(customer), I will have to do:

var customer = GetCustomer(id);
VerifyActive(customer);

consecutively at several places, and I think it counts as violation of DRY. Is this a good idea? Any idea how to help with the long method names?

Louis Rhys
  • 6,042
  • 11
  • 42
  • 59
  • the check after the call is like verifying a post condition, you should be able to assume it is correct as long as the preconditions were all true – ratchet freak May 16 '13 at 07:22

5 Answers5

3

What your methods are doing is not at all unreasonable. Its just unfortunate that their names are a bit of a mouthful.

For example where you have a method GetCustomerAndVerifyActive, surely you could call this GetActiveCustomer instead, and its easier on the eye? The new name doesn't detract from what the method actually does.

In summary I think your method names should reflect what the method does at a high level, not line-by-line. And certainly in .net you have the /// embellishments which allow you to specify what the method does in a more detailed way.

Example

/// <summary>
/// Returns an Active customer
/// </summary>
/// <param name="id">the customer's id</param>
/// <returns>the active customer</returns>
/// <remarks>Note this will throw an exception if the customer doesn't exist, or is inactive</remarks>
Customer GetActiveCustomer(int id)
{
    Customer customer = Db.GetCustomerById(id);
    if(!customer.IsActive) throw new Exception("Customer is not active");
    return customer;
}
PeteH
  • 276
  • 1
  • 5
1

There two general principles at stake here:

  • single responsibility
  • smart interfaces

It is always a good idea that one entity, method or class, have one responsibility, but the interface to any function or class should be easy to use.

If it should always be verified and especially if it does not make any sense to use the object, which you get without verification, then you should verify it within the get method.

such long names for variables are not readable and not necessary. It is very sensible to just use the getVarName and let it do the check.

CodeTower
  • 133
  • 7
0

The sole point of "one responsibility" is to make programs easier to understand for human readers. Computing a certain path and making sure it exists can be seen as two different responsibilities, but they are certainly very closely related; I don't see a risk of confusing your understanding here, particularly as long as the lookup routine stays as short as it is now.

Certainly it is better than having to remember that the path, which you will probably want to manipulate after you've retrieved it, may not exist - that would require you to write twice as much code in every place where you use the routine, at a clear net loss.

A good litmus test for such questions is: if ensuring existence isn't this routine's job, whose job is it? Any utility you would write for the purpose would first have to call the path computation routine, so that they resemble a pair of conjoined twins - neither really with any purpose except in combination with the other. It can be good practice to refactor such "twin" routines into separate methods anyway for better readability, but not when both are as tiny as these are. (Note: this is an opinion. I am aware that there are people who refactor everything away when it is longer than one line.)

I have assumed here that you do actually need to ensure the existence of this path on every access. If that isn't the case, then you really do have two different tasks:

  1. Hypothetically, if I had a , where would I put it?
  2. Give me a location where I can put this right now!

Those two would probably merit different methods.

Kilian Foth
  • 107,706
  • 45
  • 295
  • 310
0

The first two examples and the third one are quite different.

In the first two examples you first check a condition (if the path exists), and only then construct an object to return. This is absolutely acceptable.

What you do in the third example is retrieve data and check if the data is valid. I think this is over-complicated. It would be better to first check if the customer is active (maybe by adding a method to the DB layer for that) before retrieving it.

Throwing an exception if an object couldn't be created is IMHO over-complicated too. You could return null instead of throwing and exception, and then simply check for null instead of having try-catch blocks.

What comes for the methods names, you verbally describe all the steps in the methods _ retrieving, validating, checking. They can be easily simplified:

  • GetReportOutputDirectory instead of GetReportOutputDirectoryAndMakeSureExist
  • GetReportConfigurationPath instead of GetAndVerifyExistenceOfReportConfigurationPath
  • GetActiveCustomer instead of GetCustomerAndVerifyActive

The methods are usually expected to return valid data, which can be guaranteed by pre-checking a condition. So I wouldn't say that the methods do two things: they return a valid object, which is quite reasonable.

superM
  • 7,363
  • 4
  • 29
  • 38
  • In the the third example, I do this method before operations that doesn't make sense if the customer isn't active – Louis Rhys May 16 '13 at 07:47
  • The method is expected to return the customer only if it is active. The code you wrote will achieve this, but I think it should be simplified. – superM May 16 '13 at 08:12
0

I am told that it is normally a good idea for a method to do two things, or for a method to have side-effects (like creating directory).

Well, it's normal that methods do more than one thing, i.e. we got used to it, but it is not advisable (generally speaking).

In your particular examples I see a very thin line between comfort/single responsibility and readability/seperation of concerns. The major problem I see, however, is that you encoded the methods' behavior in their names. E.g. GetReportOutputDirectoryAndMakeSureExist does exactly that in code. When you change the behavior slightly (e.g. add a dummy file in every new directory), you'd have to change the method name, too.

My suggestion (and it seems I'm not the only one) is to start with changing the method-names. For example GetReportOutputDirectoryAndMakeSureExist could be renamed to initReportOutputDirectory. Then you should take a look at the surrounding of the methods - is one method really used that often around in the project? Or just within a single class? I often find another segregation makes more sense, when looking back.

Andy
  • 1,305
  • 7
  • 13