2
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ if this is going to be an aws lambda, then i think yes. \$\endgroup\$ – hjpotter92 Nov 19 '20 at 19:33
  • \$\begingroup\$ Would you please elaborate and explain why it maters how or where the code is executed? \$\endgroup\$ – OSGI Java Nov 20 '20 at 2:56
  • \$\begingroup\$ The current question title, which states your concerns about the code, applies to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$ – BCdotWEB Nov 20 '20 at 8:40
  • \$\begingroup\$ @BCdotWEB these kinds of questions and concerns are oftentimes brought up during code reviews as it was in this case. Does this explanation help or more information is needed? \$\endgroup\$ – OSGI Java Nov 20 '20 at 13:12
  • 1
    \$\begingroup\$ @OSGIJava the only solution I know is to cover code with tests to make refactoring safe. \$\endgroup\$ – Aleksei Tirman Nov 20 '20 at 15:30
3
\$\begingroup\$

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);
}
\$\endgroup\$
  • \$\begingroup\$ My response would not fit here so I added it to the very bottom of the original post. I would love to hear what you think. \$\endgroup\$ – OSGI Java Nov 20 '20 at 13:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.