I’m trying to make a utility Master class for aws client, and have mainly come across 2 types of commands/operations. Ones that require the S3Client, and others that do not. Therefore, I divided the ones that require S3Client under the S3AggrgtnClient and the other that didn’t require the S3Client into the S3AggrgtnWorker.
Using private constructor and static factory methods, I’m trying to emulate a dynamic instancing of the same base class implementing a limited set of methods only, depending on which factory method is called and parameters passed through.
Is this a good way/design pattern of doing this? If not please suggest some alternatives which are better:
import { S3Client } from '@aws-sdk/client-s3';
import { S3AggrgtnViewer } from './viewer/viewer';
import { S3AggrgtnPresigner } from './presigner/presigner';
import { S3AggrgtnUploader } from './upload/uploader';
interface S3AggrgtnClient {
get presigner(): S3AggrgtnPresigner;
get viewer(): S3AggrgtnViewer;
}
interface S3AggrgtnWorker {
get uploader(): S3AggrgtnUploader;
}
export class S3Aggrgtn {
private readonly _s3Client: S3Client | null = null;
private readonly _viewer: S3AggrgtnViewer | null = null;
private readonly _presigner: S3AggrgtnPresigner | null = null;
private readonly _uploader: S3AggrgtnUploader | null = null;
private constructor(params: { s3Client?: S3Client; file?: File }) {
if (params.s3Client) {
this._s3Client = params.s3Client;
this._presigner = new S3AggrgtnPresigner(this._s3Client);
this._viewer = new S3AggrgtnViewer(this._s3Client);
}
if (params.file) {
this._uploader = new S3AggrgtnUploader(params.file);
}
}
static getClient(s3Client: S3Client) {
if (!s3Client) {
throw Error('S3Client not provided');
}
return new S3Aggrgtn({ s3Client }) as S3AggrgtnClient;
}
static getWorker(file: File) {
if (!file) {
throw new Error('Data source not provided');
}
return new S3Aggrgtn({ file }) as S3AggrgtnWorker;
}
get uploader() {
return this._uploader;
}
get s3Client() {
return this._s3Client;
}
get presigner() {
return this._presigner;
}
get viewer() {
return this._viewer;
}
}
0
I don’t like it.
I don’t like that I don’t know how to use it unless I know how it was built.
I don’t like that despite there being a lot of code here there is no business logic. Just a lot of conditional construction.
I don’t like that if I ask for something that doesn’t exist it blows up in my face.
… 2 types of commands/operations. Ones that require the S3Client, and others that do not.
What I’d like:
S3AggrgtnClient s3ac = new S3AggrgtnClient(S3Client);
s3ac.commandOrOperationThatRequiresS3Client();
S3AggrgtnWorker s3aw = new S3AggrgtnWorker();
s3aw.commandOrOperationThatRequiresSquat();
But, I also don’t like constructors that have to do a lot of work. Set state and validate. If you want to follow the way of the OO coder consider:
S3AggrgtnClient s3ac = new S3AggrgtnClient(
new S3AggrgtnPresigner(s3Client),
new S3AggrgtnViewer(s3Client)
);
s3ac.commandOrOperationThatRequiresThingsThatRequireS3Client();
Done this way objects don’t have to know how to construct themselves. One object can be constructed many different ways. Each of those ways will need it’s own name of course. That’s so you can shove the construction code into a factory method or some such. But so long as the object gets what it needs it wont care.
Also, do it this way and once the object is built it’s ready to use. You don’t have to check. So long as it exists, it’s ready.
The intention is good, but the design guideline you’re following only makes sense from the perspective of he who built it, not he who consumes it; making it a bad design.
The design itself is flawed to the point of it being a more pressing concern than whether the factory methods themselves are implemented well.
Therefore, I divided the ones that require S3Client under the S3AggrgtnClient and the other that didn’t require the S3Client into the S3AggrgtnWorker.
Categorizing your code based on how it’s implemented, instead of what its public behavior is, means that the structure of the code will only make sense to someone who is intimately aware of the implementation details.
Imagine if doctors only prescribed specific medications. It would force patients to figure out what medication they needed, before they would be able to go to the right doctor who prescribed that medication, in order to get a prescription. That’s putting the cart before the horse. Patients can’t be expected to diagnose themselves, that’s the doctor’s job.
As a consumer, I don’t care how your class implements its logic. I care what it does for me (which is what the interface should be expressing). That’s the express purpose of encapsulation: consumers not needing to know or care about implementation details.
The way you’re structuring your code now means that as a consumer I’m not able to find the right helper until I first figure out how it is implemented in order to find the right interface that I should be looking at, at which point I’ve already done the work that your helper was supposed to save me from.
You might be thinking “I consume this helper myself, so I obviously know this”. This is the classic developer error of thinking you will (a) remember everything perfectly and (b) will never have anyone else rely on your work in the future.
Additionally, even sidestepping those issues, what this would mean is if a helper method changed its implementation without changing its public behavior, it would still be forced to migrate from one helper class to another because of a change made to a private implementation detail, which in turn would force all consumers to now have to adapt to finding the helper in a different helper class.
I get the feeling that you were too busy focusing on what patterns you could implement, that you stopped to think what patterns you actually needed to implement.
Additionally, there are several issues with the overall code quality that I would be flagging if this were a PR review:
Aggrgtndoes not meaningfully save effort over writingAggregation.- Your interfaces indicate available behavior, but your class which is the representation of this interface has a very laissez-faire attitude about whether it actually cares to perform this behavior, as evidenced by a constructor that will happily omit the necessary properties for the interface to be initialized/usable down the line
- When you inevitably then deal with absent dependencies down the line, you choose to blow up in the consumer’s face, as if it’s their fault.
- What does this class do??? It adds no logic, and instead merely forces the consumer to engage in an game of “how did the designer of this class arbitrarily decide I should access it?” with no clear indication on how it’s supposed to work, with the promise of blowing up in the consumer’s face at runtime when they didn’t perform the correct magical movements. This isn’t a helper, it’s an obfuscator.