Naming several chained functions that only do a small part of the job-CodePudding

I sometimes have this issue where a task is done by a set of functions, each of them actually just preparing the context and getting a bit closer to the original task, but only the last one is really doing the job, and naming each of them is a nightmare.

Here is an example that exhibits the naming issue:

/* This is the API, it has to be called this way. */
Stuff getStuff(Request request) {
  Stuff stuff = new Stuff();
  for(handleRepeat(request)) {
    stuff.add(reallyGetStuff(request))
  }
  return stuff;
}

Stuff reallyGetStuff(Request request) {
  logRequest(request);
  Stuff stuff = reallyReallyGetStuff(request)
  logStuff(stuff);
  return stuff
}

Stuff reallyReallyGetStuff(Request request) {
  Stuff stuff = reallyReallyReallyGetStuff(request)
  executeCallbacks(stuff);
  return stuff;
}

Stuff reallyReallyReallyGetStuff(Request request) {
  if (request.isFoo()) {
    return getFooStuff()
  } else {
    return getBarStuff()
  }
}

Calling them handleRepeatAndLogAndExecuteCallbacksAndGetStuff, logAndExecuteCallbacksAndGetStuff, executeCallbacksAndGetStuff, and getStuff would be more correct, but is obviously unacceptable.

Here is a single-function version. More readable at this scale, but not scalable at all. It mixes 5 different responsibilities (repeats, logs, callbacks, foo/bar split, and Stuff aggregation), which is the opposite of clean code.

Stuff getStuff(Request request) {
  Stuff result = new Stuff();
  for(Request singleRequest : handleRepeat(request)) {
    logRequest(singleRequest);
    if (singleRequest.isFoo()) {
      stuff = getFooStuff(singleRequest);
    } else {
      stuff = getBarStuff(singleRequest);
    }
    handleCallbacks(stuff);
    logStuff(stuff);
    result.add(stuff);
  }
  return stuff;
}

Here is the core part of a functional version. Note that the logging is split in two parts, I can't log the duration anymore for example.

Stuff getStuff(Request request) {
  return new Stuff(handleRepeat(request).stream()
      .peek(StuffReader::logRequest)
      .map(StuffReader::getSpecificStuff)
      .peek(StuffReader::handleCallback)
      .peek(StuffReader::logStuff)
      .collect(Collectors.toList()));
}

Is there a way to do organize this code closer to what the first example is doing but with a better naming? Or could you suggest any alternative solution where each method only has one responsibility and the result is still sensible and clean?

CodePudding user response：

If you are dividing the method into smaller methods, each one with its responsibility (which is always a good idea) , that means that each method does a different thing. So, just adding really to its name one or many times doesn't match the content of the method.

You said you cannot do it in one method because violating SRP due to doing different things like repeats, logs, callbacks, foo/bar split, and Stuff aggregation. So, each method should be called accordingly:

repeats iterateOverElemtns(...)
logs logRequest(...)
callbacks applyCallbacks(...)
foo/bar foo() bar()
split reduceProblem(...)
Stuff aggregation aggregateResults(...)

Actually the first single method looks pretty clean to me, I just could extract the if:

Stuff getStuff(Request request) {
  Stuff result = new Stuff();
  for(Request singleRequest : handleRepeat(request)) {
    logRequest(singleRequest);
    applyFooBarLogic();
    handleCallbacks(stuff);
    logStuff(stuff);
    result.add(stuff);
  }
  return stuff;
}

This is really clean code. You are overengineering in the first block. In the logRequest() just log the request, in the handleCallbacks(...) just handle callbacks, and so on. The getStuff(...) api method is the orchestrator, the high level action. Every business action (with one name) can be divided in smaller actions, with more specific names in a one deeper level of abstraction. If the getStuff is the higher abstraction naming, you should not use the name getStuff in the lower level.

To use an every day example, imagine you have a method eat(), that could be divided in putFoodInTheMouth(), chewFood(), swallowFood() and so on. You won't name those methods as eat(), reallyEat(), reallyReallyEat(). It's all about abstraction levels.

CodePudding user response：

How about something like this:

abstract class RequestHandler<T> {

    abstract T handleRequest(Request request);

}


public class StuffHandler extends RequestHandler<Stuff> {

    Stuff handleRequest(Request request) {
        // Do first thing
        getStuff(request)
        reallyGetStuff(request)
        reallyReallyGetStuff(request)
        reallyReallyReallyGetStuff(request)
    }

    // private doStuff / reallyDoStuff methods or whatever this particular handler needs to do
    private Stuff getStuff(Request request) { ... }

    private Stuff reallyGetStuff(Request request) { ... }

    private Stuff reallyReallyGetStuff(Request request) { ... }

    private Stuff reallyReallyReallyGetStuff(Request request) { ... }
}

this is clean. each individual handler is "clean" when observed from outside, and has only one responsibility - handling his thing. How it handles it, however, up to you, but there is a way to make this convoluted thing clean as well.

But i believe, once this abstraction is in place, that cleaning up individual methods of individual handlers will become easier; and becomes less of a factor. Some handlers will be simple, some others like this one might still be convoluted; but from the outside it's just a single thing with a single responsibility. Internal handler's implementation becomes a less important thing, as cleaning up (and dividing/following the "single responsibility principle" is scoped to just that particular handler)

Of course, you get additional bonus points by making the abstract handleRequest method have an abstract implementation that satisfies most cases, and leave the edge-case handlers to separate out the convoluted logic into new methods.

At this point, you'll definitely know whether just one abstract RequestHandler is enough, or you need more, e.g.

abstract ConvolutedRequestHandler<T> extends RequestHandler<T> {

   T handleRequest(Request request) { 
     // figure out how to "standardize" this convolution
      logAndRequestStuff(request)
      T stuff = getStuffFromRequest(request)
      return stuff.getFooOrBarStuff(request)
   }

   // offer additional abstract methods that these convoluted handlers will need
   abstract logRequestAndStuff(Request request)

   abstract T getStuffFromRequest(Request request)

   abstract T getFooOrBarStuff(Request request)
}

abstract TypicalRequestHandler<TypicalThing> extends RequestHandler<TypicalThing> {


   TypicalThing handleRequest(Request request) {
       // call into RequestHandler::handleRequest which catches most cases fine
       super.handleRequest(request)
   }

this is roughly modeled on your rough example, so it makes as much sense as the original code you posted.

But the idea is to generally get rid of getFooOrBarStuff method, and just use an appropriate handler for each of the requests. Ideally, fetch (or have a factory) for the appropriate Handler for each of the request-types you have.

and externally, just do this:

        RequestHandlerFactory<T> factory = new RequestHandlerFactory<T>();
        // getClass() here referring to the Request / RequestThing or even the thing that drives the request handlers themselves
        T requestHandler = factory.buildRequestHandlerFor(getClass());
        ? handlerResult = requestHandler.handleRequest(request)

but that really depends on what you pick your generic argument to be, whether the type of request, the type of the request's return value, or you know.