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

Antipattern of variadic New factory #27

Open
gabizou opened this issue Feb 6, 2025 · 1 comment
Open

Antipattern of variadic New factory #27

gabizou opened this issue Feb 6, 2025 · 1 comment
Labels

Comments

@gabizou
Copy link

gabizou commented Feb 6, 2025

The New constructor feels misleading from the initial reading, assuming that the options would be consumed as a whole, and not ignored. In many go examples, a variadic argument should be consumed by default.

func New(options ...Options) (*Sqids, error) 

What I'd suggest is a break, but a worthwhile one, where the Options type becomes a function operating on an Option that sequentially get applied (and remains true with the default setting), so that consumers can do something like in this playground (copying here for persistence)

type Option struct {
	Alphabet  string
	MinLength uint8
	Blocklist []string
}

// Options for a custom instance of Sqids
type Options = func(*Option)

// Sqids lets you generate unique IDs from numbers
type Sqids struct {
	alphabet  string
	minLength uint8
	blocklist []string
}

func WithAlphabet(alphabet string) Options {
	return func(o *Option) {
		o.Alphabet = alphabet
	}
}

func WithBlocklist(list []string) Options {
	return func(o *Option) {
		o.Blocklist = list
	}
}

func AddToBlocklist(blocked ...string) Options {
	return func(o *Option) {
		o.Blocklist = append(o.Blocklist, blocked...)
	}
}

func validateOptions(o *Option) (*Option, error) {
	return o, nil
}

// New constructs an instance of Sqids
func New(options ...Options) (*Sqids, error) {
	o := &Option{
		Alphabet:  defaultAlphabet,
		Blocklist: defaultBlocklist,
	}
	for _, opt := range options {
		opt(o)
	}
	o, err := validateOptions(o)
	if err != nil {
		return nil, err
	}

	return &Sqids{
		alphabet:  o.Alphabet,
		minLength: o.MinLength,
		blocklist: o.Blocklist,
	}, nil
}
@4kimov
Copy link
Member

4kimov commented Feb 17, 2025

@gabizou Thanks for the suggestion. I'll keep this open for now, so we can revisit next time we plan a breaking release.

@4kimov 4kimov added enhancement New feature or request open for discussion labels Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants