Is this an excessive abstraction using objects?

Question

We have two physical AWS S3s called in the code s3Legacy and s3Cache. They store the same data but with different S3 key structures.

The fists solution shared below uses objects, the second does not.

Is the solution using objects an excessive abstraction or there are benefits to it? (There is no need to comment on the use of the callback.)

Here is a version using objects:

// not all code is shown here for the sake of brevity 
module.exports.handle = (event, context, cb) => {

    const s3Sdk = new AWS.S3({ region: config.s3.region });
    const s3Legacy = new S3(s3Sdk, config.s3.legacyBucketName);
    const s3Cache = new S3Cache(s3Sdk, config.s3.cacheBucketName);
    
    s3Legacy.putObject(key, event.body, callback);
    s3Cache.putObject({
                unitId: event.pathParameters.unitId,
                timestamp: event.timestamp,
                eventId: event.id,
                eventFileName: event.id,
                body: event.body
            }, callback)'

}

class S3 {
    constructor(s3Sdk, bucketName) {
        this.s3Sdk = s3Sdk;
        this.bucketName = bucketName;
    }

    putObject(key, body, callback) {
        this.s3Sdk.putObject({ Bucket: this.bucketName, Key: key, Body: body }, (err, response) => {
            callback(err, response);
        });
    }
}

class S3Cache extends S3 {

    constructor(s3Sdk, bucketName) {
        super(s3Sdk, bucketName);
    }

    putObject(params, callback) {
        const key = this.toS3Key(
            params.unitId,
            this.toTimestampObject(params.timestamp),
            params.eventId,
            params.eventFileName
        );
        super.putObject(key, params.body, callback);
    }

    toS3Key(unitId, incidentTimestamp, incidentId, archiveIncidentObjectName) {
       return ....
    }

    toTimestampObject(timestamp) {
        return ....
    }
}

Here is a version without using objects:

// not all code is shown here for the sake of brevity 
module.exports.handle = (event, context, cb) => {

    const s3 = new AWS.S3({ region: config.s3.region });
    await s3.putObject({ Bucket: config.s3.legacyBucketName, Key: key, Body: event.body }).promise();
    await s3.putObject({
            Bucket: config.s3.cacheBucketName,
            Key: toS3Key(event.unitId, toTimestampObject(event.timestamp), event.eventId, event.eventId),
            Body: event.body
        }).promise();
}

function toS3Key(unitId, incidentTimestamp, incidentId, archiveIncidentObjectName) {
       return ....
}

function toTimestampObject(timestamp) {
        return ....
}

In this version of the code, both the service orchestration code (write to s3Legacy and if successful write to s3Cache) and the s3Legacy, s3Cache, the code constructing the S3 key resides in one source file.

So this source file could change for multiple unrelated reasons. For example when we want to change the orchestration, change the order of writing to S3s or if we want to write to DynamoDB for example. The source file would also change if we need to change the S3 key schema.

We set a pattern here that every new addition would be added to this single source file which size would grow as a result and the list of reasons for a change would also grow.

While this example here is still small, calls only 2 services, I wonder whether extracting out the S3 code would set a better starting point for additional new development and enhancement.

Answer 1

I think the use of classes here is excessive because of complexity added by their structure and inheritance. Since your objects have only one method putObject you can make abstraction on the function level. So you can just use closures to save state by returning a function that will put objects. In this case s3Cache will use only one function of s3Legacy as a dependency.

Here is the code:

const s3Legacy = (sdk, bucket) => function putObject(key, body, callback) {
    sdk.putObject({ Bucket: bucket, Key: key, Body: body }, callback);
}

const s3Cache = (putObjectLegacy) => function putObject(params, callback) {
    const toS3Key = (unitId, incidentTimestamp, incidentId, archiveIncidentObjectName) => 'implementation';
    const toTimestampObject = (timestamp) => 'implementation';

    const key = toS3Key(
        params.unitId,
        toTimestampObject(params.timestamp),
        params.eventId,
        params.eventFileName
    );

    putObjectLegacy(key, params.body, callback);
}

module.exports.handle = (event, context, cb) => {
    const s3Sdk = new AWS.S3({ region: config.s3.region });
    const putObjectLegacy = s3Legacy(s3Sdk, config.s3.legacyBucketName);
    const putObjectCache = s3Cache(s3Legacy(s3Sdk, config.s3.cacheBucketName));

    putObjectLegacy(key, event.body, callback);
    putObjectCache({
        unitId: event.pathParameters.unitId,
        timestamp: event.timestamp,
        eventId: event.id,
        eventFileName: event.id,
        body: event.body
    }, callback);
}