Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new approach for builder pattern #15

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

miladvafaeifard
Copy link

feel free to make suggestion and improvements based on the approach.

@miladvafaeifard miladvafaeifard force-pushed the builder-design-pattern-improvement branch from 9194068 to 5530529 Compare October 16, 2018 11:22
@miladvafaeifard miladvafaeifard force-pushed the builder-design-pattern-improvement branch from 5530529 to 5f9beda Compare October 16, 2018 13:05
@miladvafaeifard
Copy link
Author

@torokmark, have you checked my suggestion of the changes? :)

@torokmark
Copy link
Owner

Hi Milad,

Nice approach. Though, I am a bit concerned about the followings:

  1. Even though, Build interface is a general type with type param, it requires the implementor class to have setAge, setPhone, setAddress methods.
    What if I am intended to use Build with other type paramater like Car. I cannot use it, since it makes me implement setPhone in Car, but a car does not have phone number.
    In my point of view, Build is quite misleading in this way.

    Let me suggest the following.

    Let us have a Build interface with type parameter, and this interface has only one method, like build(): T

namespace BuilderPattern {
    export interface Builder<T> {
        build(): T;
    }

    // ...
}
  1. Director with UserBuilder parameter in its constuctor is a bit smelly in this way. Why does not makeUser return a User?

I would recommend the following here:

export class UserDirector implements Director<User> {

    constructor(private userBuilder: UserBuilder){}


    create(): User {
        if(this.userBuilder.Name === 'Jancsi') {
            this.userBuilder.setAge(12)
            .setPhone("0123456789")
            .setAddress("asdf");
        } else {
            this.userBuilder.setAge(0)
            .setPhone("xxxxxxxxx")
            .setAddress("xxxx");
        }
        return this.userBuilder.build()
    }
}

@miladvafaeifard
Copy link
Author

miladvafaeifard commented Jun 2, 2020

Hi Mark 👋

Your approach makes much more sense to me, hence I completely agree on this. Shall i refactor with your approach in my next commit in this thread with a quick polish? 😃

@torokmark
Copy link
Owner

@miladvafaeifard , yes please, it would be great if you did refactoring on the approach.

I am always happy to see people contributing. Thank you

@miladvafaeifard
Copy link
Author

Hi @torokmark,

I refactored and polished based on your suggestion. I would appreciate if you could review the change. and provide your thoughts on this.

return this;
}

getUser() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I see, getUser is not used, and seems superfluous, since we have a build method.

this.name = name;
}

setAge(value: number): UserBuilder {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a bit misleading to have getter by declaring with get and setters following Java POJO.
Here, methods should be introduced with set keyword.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants