There is a popular idea that refactoring is making code shorter. It is not entirely wrong. Repetition, pointless conditional branches, and dead variables do make code longer for no reason. But the usual topmost problems are when the intentions of a class's main public method, the business or operational ones, are unclear to a reader.

I had such a case in an S3 document-upload adapter.

The adapter was not doing anything unusual. It had to stream a document to S3-compatible storage, count actual bytes, calculate a checksum, reject empty or too-large content, normalize S3 errors, clean up a stored object where needed, and write technical logs. This is a substantial piece of work. The problem was that the public store() method contained most of it directly.

At a glance it looked like a method that stores a document. In practice, reading it meant walking through AWS SDK setup, try/catch blocks, S3 error-object construction, cleanup, and a large logging payload before reaching the returned value.

The code was not unreadable because it had many lines. It was unreadable because the important lines had no space to stand out.

The Main Method Refactor

The main method did not show the adapter's use case. Before the refactoring, upload execution was part of store() itself:

try {
 resp new Upload({
  client: this.#s3_client,
  params: {
   Bucket: this.#bucket_name,
   Key: documentStorageKey,
   Body: countedStream,
   ContentType: file.mime_type
  },
  leavePartsOnError: false
 }).done();
} catch (error: unknown) {
 // Detect validation errors.
 // Build an S3 error object.
 // Build an application exception.
 // Log it.
 // Throw it.
}

How do I decide what to refactor? ne rule of thumb is to watch the nesting depth. In my book, a class should not contain more than three levels of nesting. The first is the class declaration, the second is the method level, and the third is at most one level of nesting within a method - inline object literals and deeply nested objects are not allowed.

The second rule - the main public methods must reveal their intention clearly by their names and their internal code must reveal the story of how the method does what it does in the domain or integration terms.

So the first refactoring was to extract that try/catch into a story-telling method:

const resp this.uploadToStorage(documentUUID, documentStorageKey, file, countedStream, startedAt);

It was "Extract Method" refactoring, described by Martin Fowler in his Refactoring book. The main method started to read as a story because the implementation was hidden and the method name revealed intention. It packed 40+ lines into one intention-revealing call — roughly 40x less implementation detail to scan in the main flow.

I used the same approach to extract two conditionals converting a raw S3 error into an application error, and to delete an empty object and throw another error. The first turned ~20 lines into one call, and the second reduced another ~30 lines to one call. Together, roughly 90 lines of implementation detail became three intention-revealing calls — about 30x less code to scan at this level. And look at the method names — they continue the story

this.throwIfS3Error(storedObjectKey, response, httpStatusCode, startedAt);
await this.throwIfUploadedFileIsEmpty(storedObjectKey, file, countedStream, startedAt);

Moreover, now a reader can answer the first useful question quickly: what happens when a document is stored? It is uploaded. The response is checked. Empty content and errors are dealt with.

The helper methods still contain difficult code. That is fine. Difficult code should exist where it belongs. It should not hide the business flow of the public method.

[!INFO]
The main store() method shrank from 121 lines to 39 — a 68% or 3x reduction in code to read.

A Private Helper Is Not Automatically a Public API

The next request looked small. The upload service needed to delete the object it had just stored when a duplicate document was found, or when persistence failed later in the transaction.

There was already a private cleanup method.

private async cleanupStoredObject(documentUUID: string, documentStorageKey: string, startedAt: number): Promise<void> {}

A tempting change was to make it public. That would have been wrong.

The private method accepted a document UUID, a storage key, and a timestamp. The timestamp existed only because the method wrote a cleanup-failure log. It also swallowed a deletion error. That was correct for the empty-upload path: the caller must receive the original “empty upload” error even when cleanup itself fails.

It is not a public removal contract. The public operation became this instead:

public async remove(storedObjectKey: string): Promise<void>

It now owns the S3 request. It owns the timing of the S3 operation. It normalizes the provider error. It writes the technical storage event. It throws the normalized error to its caller and it is universally used by the adapter and its caller.

public async remove(storedObjectKey: string): Promise<void> {
    const removalStartedAt = Date.now();

    try {
        await this.#s3_client.send(new DeleteObjectCommand({
            Bucket: this.#bucket_name,
            Key: storedObjectKey
        }));
    } catch (error: unknown) {
        const normalizedFailure = this.normalizeStorageFailure(error);

        this.logStorageEvent({
            logLevel: ELogLevel.ERROR,
            eventName: EDocumentStorageLogEventName.DOCUMENT_STORAGE_CLEANUP_FAILED,
            outcome: ELogOutcome.FAILURE,
            operationName: DOCUMENT_STORAGE_LOG_CONSTANTS.operation.remove,
            storedObjectKey,
            startedAt: removalStartedAt,
            error: normalizedFailure
        });

        throw normalizedFailure;
    }
}

The caller decides what to do with that error.

The remove() itself catches it for an empty upload because empty content remains the reported result. The adapter's caller - the upload service - will catch it for a duplicate upload because a duplicate remains the domain concern, not the adapter's one. These are different decisions made by different owners.

One String, Two Names

Naming was not cosmetic in this refactoring. The application has a document UUID. The S3 adapter uses an object key. They were used in the multiple adapter logging methods extracted from the main store() method.

private logFailure(documentUUID: string, documentStorageKey: string, error: MinimalRAGException, startedAt: number): void {}

The document UUID is part of the key:

const storedObjectKey = `raw/${documentUUID}`;

The domain value object returns it as document_storage_key, because it is document metadata from the domain side. Inside the S3 adapter, it is storedObjectKey, because it is passed to the AWS SDK as Key.

The difference matters most when deleting an object.

At one point, removal was proposed as remove(documentUUID), with the adapter reconstructing raw/<UUID>. It looked tidy. But the exact key is already known after store() returns. Rebuilding it makes removal depend on a key layout which may later change. Passing the known stored object key is simpler and more honest.

The final refactoring replaces multiple log methods with one (Parameterize Method), parameterized with an object instead of a long list of positional parameters (Introduce Parameter Object). The method signature becomes 2x shorter, with intention-revealing names that are clear at first sight.

private logStorageEvent(options: TLogStorageEventOptions): void {}

Logging Had Become Its Own Mini-Application

This refactoring describes the other dimension of the refactoring result shown above.

The adapter had a method for successful storage. Another for successful removal. Another for S3 failure. Another for validation failure. Another for cleanup failure.

They all built nearly the same event:

{
    event_name,
    event_type: ELogEventType.INTEGRATION,
    outcome,
    resource: { storage_key: storedObjectKey },
    operation,
    error_code?,
    error?
}

Different logging events are useful. Five separate ways to construct 70%-identical logging event are not.

private logSuccess(documentUUID: string, documentStorageKey: string, startedAt: number, actualByteSize: bigint, httpStatusCode: number | undefined): void {}

This is where a long argument list becomes a real problem. Six positional arguments might compile. Nobody wants to remember their order during a production incident.

The final code uses the single logging method and one parameter object:

type TLogStorageEventOpti & {
 logLevel: ELogLevel;
 outcome: ELogOutcome;
 error?: MinimalRAGException;
};

// ...

this.logStorageEvent({
 logLevel: ELogLevel.WARN,
 eventName: EDocumentStorageLogEventName.DOCUMENT_STORAGE_VALIDATION_FAILED,
 outcome: ELogOutcome.FAILURE,
 operationName: DOCUMENT_STORAGE_LOG_CONSTANTS.operation.store,
 storedObjectKey,
 startedAt,
 actualByteSize: countedStream.actual_byte_size,
 error: rejection
});

The refactoring reduced roughly 90 lines the reader previously had to scan to about 30.

Deeply Buried Intention

The builder of the logged object initially contained code like this, which is completely opaque:

{
    operation: new LogOperationVO({
        // other fields
        ...(cause?.request_id ? { provider_request_id: cause.request_id } : {})
    }),
}

That conditional object spread was very far from readable.

I extracted values into interim constants with intention-revealing names and used them in the logged object construction:

// Extract all ternaries to the clearly named constants
const actualByteSize = options.actualByteSize?.toString() ?? null;
const httpStatusCode = options.httpStatusCode ?? cause?.http_status_code ?? null;
const providerRequestId = cause?.request_id ?? null;
const storageError = cause
 ? new LogErrorVO({
    name: cause.error_name,
    message: cause.error_message,
    dependency: DOCUMENT_STORAGE_LOG_CONSTANTS.dependency
   })
 : null;

// Call the logger
this.#logger.log(options.logLevel, {
 operation: new LogOperationVO({
  // Other fields
  actual_byte_size: actualByteSize,
  http_status_code: httpStatusCode,
  provider_request_id: providerRequestId
 }),
 error: storageError
});

This makes the final log event construction boring. That is good. Logging is repeated often enough throughout an application that it should better be DRY.

Actual Refactorings Used

For readers who care about Fowler’s refactoring names, here is the short record.

Make The Public Storage Flow Readable

Clarify Storage Vocabulary and Logging

The public remove(storedObjectKey) method is not a Fowler refactoring. It is a new adapter capability. The validation event rename is not one either. It changes the meaning of the logged outcome. Calling every improvement a refactoring only makes the word less useful.

Result

The adapter did not become magically small. It should not. It now does more than it did before: it exposes removal, logs useful operational information, returns a named S3 response type, and gives the upload service a clean cleanup operation.

And the public path is now readable as a path:

const resp this.uploadToStorage(...);
this.throwIfS3Error(...);
await this.throwIfUploadedFileIsEmpty(...);
this.logStorageEvent(...);

That is the result I want from refactoring - saving the reader's cognitive capacity for the really important work: deciding what, when and how the application should do in business and integration terms.

NB: The initial code implementation I had to refactor was written by AI.

🔗 Nguồn bài viết gốc: Valentine Shi