Clean Code: can private methods rely on member variables set by other private methods?

Question

I was asked to create a class that will connect to a remote service via soap. The class has only one public method send(recordName).

The class ended like this:

private recordName;
private recordId;

public send(recordName)
{
    this.recordName = recordName;
    this.validateRecordName();
    this.retrieveRecordId();

    if (this.recordExists()) {
        this.updateRecordOnRemoteServer();
    } else {
        this.addRecordOnRemoteServer();
    }
}

private validateRecordName()
{
   // throws exception if this.recordName does not meet certain criteria
}

private retrieveRecordId()
{
    response = // creates a soapClient, sends the recordName, and returns  a response
    this.recordId = response->id ? response->id : null;
}

private recordExists()
{
    return this.recordId !== null;
}

private updateRecordOnRemoteServer()
{
    someSoapClient->update('url', this.recordId); // notice the this.recordId reference
}

// omitting add method for brevity.

But my boss (who has like 10 years more experience than me), told me I should pass the recordId as a parm to the update method, because otherwise the class suffers from a Orders Matter issue, and the update method is fragile, since it requires data that is set outside itself (retrieveRecordId()), and that has to be invoked by other method (in this case send()).

So the changes are really slight:

public send(recordName)
{
    // unchanged
    this.recordName = recordName;
    this.validateRecordName();

    // changed. Notice recordId is now a method var, not a class member.
    recordId = this.retrieveRecordId() // returns the id or null
    recordExists = recordId !== null;

    if (recordExists) {
        // notice the param is being passed
        this.updateRecordOnRemoteServer(recordId);
    } else {
        // add new record
    }
}

I read Clean Code by Robert C. Martin, and most of his examples ended similar to what I proposed originally. He will initialize class memeber variables in the public method by invoking private methods, and those attributes will be used by other private methods; all in cascade.

When I first read the book I wasn't convinced it was "that clean" because I also thought that the class relied heavily on the invocation order of the private methods; but I started using that pattern in my own projects and I found it was helping me a lot to keep my code shorter and my public methods super readable.

I also learnt from the book to avoid passing params to private methods (unless there is no other way like sum(a, b), sub(a, b) kind of functions). 95% of the time he won't define arguments for private methods.

I would really like to know if there is a better approach. Thank you!

Answer 1

recordName and recordId are initialized and used during the execution of send, but they have existed before send was invoked and will continue to exist after send is done. Also, if send is called multiple times on the same object from different threads, the threads will mutate the same memory even though each needs different values, creating data races. It's not fun to have to synchronize threads that don't need to use the same data...

This is very similar to using global arguments - though the fact that they are private members touched by private methods helps containing the problem. The only reason Orders Matter is not an issue is that the class is small enough to easily notice when it raises problems. If the class gets bigger, it will become an issue.

When you have a subset of private methods that use a subset of private members, that usually means you need another class:

class Record {
    private recordName;
    private recordId;

    public Record(recordName) {
        this.recordName = recordName;

        validateRecordName();

        this.recordId = retrieveRecordId();
    }

    private retrieveRecordId()
    {
        response = // creates a soapClient, sends the recordName, and returns  a response
        return response->id ? response->id : null;
    }

    private validateRecordName()
    {
        // throws exception if this.recordName does not meet certain criteria
    }

    public recordExists()
    {
        return this.recordId !== null;
    }
}

public send(recordName)
{
    record = new Record(recordName);

    if (record.recordExists()) {
        this.updateRecordOnRemoteServer(record);
    } else {
        this.addRecordOnRemoteServer(record);
    }
}

private updateRecordOnRemoteServer(record)
{
    someSoapClient->update('url', record.recordId);
}

Answer 2

I read Clean Code by Robert C. Martin, and most of his examples ended similar to what I proposed originally

Yes, that is his style. As far as I'm concerned he is giving bad advice which you should stop following right now.

For me, your version makes it unclear how things inter-relate.

public send(recordName)
{
    this.recordName = recordName;
    this.validateRecordName();

Does this validate against the recordName I just stored? Probably, but I can't tell from here.

    this.retrieveRecordId();

Does this depend on the validateRecordName() somehow? Or does it just depend on the recordName being stored? I can't tell.

    if (this.recordExists()) {

Is this a reference to the id not being found? Or is it a reference to the name not being found?

        this.updateRecordOnRemoteServer();
    } else {
        this.addRecordOnRemoteServer();
    }

What does these update/add? Do they set the name? Will my recordId be set afterwards? I don't know.

}

You version doesn't let me know how the different pieces relate unless I go an read them. I think this seen as readable by some, because your version looks a lot like a natural language description of the process. I think thats a bad idea because natural languages suck. I want a language that makes it explicit how everything relates.

The fact is, I don't think you far enough in your non-Robert Martin version. You still stored recordName on the instance, and you simply shouldn't do that.

public send(recordName)
{
    this.validateRecordName(recordName);

    recordId = this.retrieveRecordId(recordName) // returns the id or null
    recordExists = recordId !== null;

    if (recordExists) {
        // notice the param is being passed
        this.updateRecordOnRemoteServer(recordId);
    } else {
        this.addNewRecordOnRemoteServer(recordName);
    }
}

Now I can tell how state moves between my different functions.

My rule is that you should only put things at the object level if they have the same lifetime as the object. Anything that is only meaningful for the lifetime of the send call should not be put on the object. Only things like connections to the server or perhaps a name -> id cache belong as a member instance.

Answer 3

The problem is that you have a mutable class and private methods that act upon the state of the class. Which means you have no real guarantee of what the state is at any particular point in time.

You should either ditch the methods that modify the state or initialize the state by passing the recordName into the constructor, and having the modifications apply to a particular instance.