Thanks, @sagiomc. Here are my suggestions:

1. Don't allow Value Types to be instantiated with invalid data

Your code uses value types like UniqueEntityID which is great. But it does not prevent anyone from constructing those types using bad data. Instead, it asks consumers of the type to perform validation. This is dangerous and can lead to bugs and/or a lot of duplicate code.

Consider the code listing for UniqueEntityID:

export class UniqueEntityID extends Identifier<string | number> {
  public constructor(id?: string | number) {
    super(id ? id : uuidv4());
  }

  public isValidId(): boolean {
    return TextUtils.isValidUuid(this.toString());
  }
}

Nothing stops us from writing:

let id = new UniqueEntityID(" ");

This means everywhere we use an instance of UniqueEntityID we have to make sure we call isValidId() on it. That's a lot of duplication all over our codebase. If we happen to forget to call isValidId(), we may find ourselves with a bug where code is expecting an instance of UniqueEntityID to be a valid UniqueEntityID and it's not.

In general, a Type should serve as a contract that all instances conform to. So, I would rewrite the UniqueEntityID (and all other Value Types) this way:

export class UniqueEntityID extends Identifier<string | number> {

  public static of(id: string) {
    UniqueEntityID.validate(id)
    return new UniqueEntityID(id);
  }

  public static next() {
    return new UniqueEntityID(uuidv4());
  }

  protected constructor(id?: string | number) {
    super(id);
  }

  protected static validate(id: string) {
    if(!UniqueEntityID.isValid(id)) {
      throw new TypeError(`Invalid id.`);
    }
  }

  public static isValid(id: string): boolean {
    return TextUtils.isValidUuid(id);
  }
}

This way, it's impossible to create an invalid instance of UniqueEntityID and any code that receives a UniqueEntityID can trust that it's valid without having to validate it. Meanwhile, if someone wants to determine whether or not a string is a valid UniqueEntityID they can do that before creating an instance.

Author and Hash also seem like good candidates for value types since they have discrete rules. Overall, this is a very powerful approach and would lead to a cleaner Comment entity since most of the validation can occur before the constructor is even called.

As a side note, importing the uuid library here is violating the Dependency Rule from Clean Architecture. In a real-world JavaScript/TypeScript application, as long as this is the only place where that library gets imported, then it's probably an okay trade-off until UUID's make it into the Language itself. Bringing MD5 directly into the Comment entity on the other hand makes me a bit nervous, I think this points to a flaw in my own implementation as well. I don't like bringing services into domain objects. I think the hash function belongs in the Use Case where the MD5 library can be injected.

And finally, as a super picky, additional side note, "UniqueEntityID" is a very long name for something that's likely to get a lot of use. How about UniqueID or even just ID?

2. Omit the word "get" from readonly property names.

In general, it's good to have function names start with verbs, and property names start with nouns. But getters are a bit odd; they let us define a function that gets invoked through property access. So, for getters, I recommend using property naming conventions over function naming conventions. As a concrete example, in the Comment class, I would rename getAuthor to just author. So...

// current code
public get getAuthor(): string {
  return this.author;
}

// suggested code
public get author(): string {
  return this.author;
}

This allows consumers to write code like:

let comment = Comment.create(...);
let author = comment.author

This feels more natural to me. The reason I used getAuthor in my example is that I tend to avoid getters in plain JavaScript so getAuthor was a typical function and was called the typical way; getAuthor(). In Typescript with Classes, I like getters just fine because your code won't compile if you try to assign a value.

3. Take advantage of `import type`

If you use import type instead of import when importing a module that only contains type information, it guarantees that the import will be removed at compile time. This avoids some subtle bugs.

And more importantly, if you import a class and only end up using it's type information (as in AddCommentRepository.ts) you ensure no unnecessary code is pulled in and avoid potentially circular references.

Hope that's helpful.

Thanks again for all your work. The code is really coming along nicely.

Edit: typos.

1. Don't allow Value Types to be instantiated with invalid data

2. Omit the word "get" from readonly property names.

3. Take advantage of import type

3. Take advantage of `import type`

typescript version #25

Description

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions