-
Notifications
You must be signed in to change notification settings - Fork 0
⚡ Optimize WordsToFormattedCase with strings.Builder to reduce allocations #20
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
base: main
Are you sure you want to change the base?
Changes from all commits
3b4d283
93056cb
1a958fe
e9505cf
81f1144
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,14 @@ func (w AcronymWord) String() string { return string(w) } | |
| func (w UpperCaseWord) String() string { return strings.ToUpper(string(w)) } | ||
| func (w SeparatorWord) String() string { return string(w) } | ||
|
|
||
| // Len implementations | ||
| func (w SingleCaseWord) Len() int { return len(w) } | ||
| func (w FirstUpperCaseWord) Len() int { return len(w) } | ||
| func (w ExactCaseWord) Len() int { return len(w) } | ||
| func (w AcronymWord) Len() int { return len(w) } | ||
| func (w UpperCaseWord) Len() int { return len(w) } | ||
| func (w SeparatorWord) Len() int { return len(w) } | ||
|
|
||
| func performCaseFirst(s string, fn func(rune) rune) (string, rune, bool) { | ||
| if s == "" { | ||
| return s, 0, true | ||
|
|
@@ -216,79 +224,174 @@ func WordsToFormattedCase(words []Word, opts ...any) (string, error) { | |
| cfg.firstUpper = true | ||
| } | ||
|
|
||
| result := make([]string, 0, len(words)) | ||
| delimiter := cfg.delimiter | ||
| if cfg.upperIndicator != "" { | ||
| if cfg.upperIndicator == cfg.delimiter { | ||
| delimiter = cfg.delimiter + cfg.delimiter | ||
| } else { | ||
| delimiter = cfg.upperIndicator | ||
| } | ||
| } | ||
|
|
||
| size := 0 | ||
| for _, word := range words { | ||
| var w string | ||
| if l, ok := word.(interface{ Len() int }); ok { | ||
| size += l.Len() | ||
| } else { | ||
| size += 5 // fallback | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jules this whole thing is a little strange since we are doing the same in every one. It is possible that we would want to multiply it such as AcronymWord might be
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that the repetition was verbose and potentially confusing given the possibility of differing lengths (e.g. for Acronyms). I have refactored this by adding a
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jules let's just make it return an error rather than a fallback, this means adding err to the function returns and creating a global function for it and handling it. |
||
| } | ||
| } | ||
| size += len(delimiter) * max(0, len(words)-1) | ||
|
|
||
| var b strings.Builder | ||
| b.Grow(size) | ||
|
|
||
| for i, word := range words { | ||
| if i > 0 { | ||
| b.WriteString(delimiter) | ||
| } | ||
|
|
||
| switch word := word.(type) { | ||
| case SingleCaseWord: | ||
| w = string(word) | ||
| s := string(word) | ||
| if cfg.allUpper || cfg.screaming { | ||
| w = strings.ToUpper(w) | ||
| for _, r := range s { | ||
| b.WriteRune(unicode.ToUpper(r)) | ||
| } | ||
| } else if cfg.allLower || cfg.whispering { | ||
| w = strings.ToLower(w) | ||
| for _, r := range s { | ||
| b.WriteRune(unicode.ToLower(r)) | ||
| } | ||
| } else if cfg.caseMode == CMAllTitle { | ||
| w = UpperCaseFirst(strings.ToLower(w)) | ||
| first := true | ||
| for _, r := range s { | ||
| if first { | ||
| b.WriteRune(unicode.ToUpper(r)) | ||
| first = false | ||
| } else { | ||
| b.WriteRune(unicode.ToLower(r)) | ||
| } | ||
| } | ||
| } else { | ||
| w = strings.ToLower(w) | ||
| for _, r := range s { | ||
| b.WriteRune(unicode.ToLower(r)) | ||
| } | ||
| } | ||
| case ExactCaseWord: | ||
| w = word.String() | ||
| s := string(word) | ||
| if cfg.mixCaseSupport { | ||
| w = splitMixCase(w, cfg.delimiter) | ||
| } | ||
| if cfg.allUpper || cfg.screaming { | ||
| w = strings.ToUpper(w) | ||
| } else if cfg.allLower || cfg.whispering { | ||
| w = strings.ToLower(w) | ||
| for j, r := range s { | ||
| if j > 0 && unicode.IsUpper(r) { | ||
| if cfg.allUpper || cfg.screaming { | ||
| for _, dr := range cfg.delimiter { | ||
| b.WriteRune(unicode.ToUpper(dr)) | ||
| } | ||
| } else if cfg.allLower || cfg.whispering { | ||
| for _, dr := range cfg.delimiter { | ||
| b.WriteRune(unicode.ToLower(dr)) | ||
| } | ||
| } else { | ||
| b.WriteString(cfg.delimiter) | ||
| } | ||
| } | ||
| if cfg.allUpper || cfg.screaming { | ||
| b.WriteRune(unicode.ToUpper(r)) | ||
| } else if cfg.allLower || cfg.whispering { | ||
| b.WriteRune(unicode.ToLower(r)) | ||
| } else { | ||
| b.WriteRune(r) | ||
| } | ||
| } | ||
| } else { | ||
| if cfg.allUpper || cfg.screaming { | ||
| for _, r := range s { | ||
| b.WriteRune(unicode.ToUpper(r)) | ||
| } | ||
| } else if cfg.allLower || cfg.whispering { | ||
| for _, r := range s { | ||
| b.WriteRune(unicode.ToLower(r)) | ||
| } | ||
| } else { | ||
| b.WriteString(s) | ||
| } | ||
| } | ||
| case FirstUpperCaseWord: | ||
| w = word.String() | ||
| if cfg.mixCaseSupport { | ||
| w = splitMixCase(w, cfg.delimiter) | ||
| } | ||
| s := string(word) | ||
| if cfg.allUpper || cfg.screaming { | ||
| w = strings.ToUpper(w) | ||
| for _, r := range s { | ||
| b.WriteRune(unicode.ToUpper(r)) | ||
| } | ||
| } else if cfg.allLower || cfg.whispering { | ||
| w = strings.ToLower(w) | ||
| for _, r := range s { | ||
| b.WriteRune(unicode.ToLower(r)) | ||
| } | ||
| } else { | ||
| first := true | ||
| for _, r := range s { | ||
| if first { | ||
| b.WriteRune(unicode.ToUpper(r)) | ||
| first = false | ||
| } else { | ||
| b.WriteRune(unicode.ToLower(r)) | ||
| } | ||
| } | ||
| } | ||
| case AcronymWord: | ||
| w = word.String() | ||
| s := string(word) | ||
| if cfg.screaming { | ||
| w = strings.ToUpper(w) | ||
| for _, r := range s { | ||
| b.WriteRune(unicode.ToUpper(r)) | ||
| } | ||
| } else if cfg.whispering { | ||
| w = strings.ToLower(w) | ||
| for _, r := range s { | ||
| b.WriteRune(unicode.ToLower(r)) | ||
| } | ||
| } else if cfg.caseMode == CMAllTitle { | ||
| w = UpperCaseFirst(strings.ToLower(w)) | ||
| first := true | ||
| for _, r := range s { | ||
| if first { | ||
| b.WriteRune(unicode.ToUpper(r)) | ||
| first = false | ||
| } else { | ||
| b.WriteRune(unicode.ToLower(r)) | ||
| } | ||
| } | ||
| } else { | ||
| b.WriteString(s) | ||
| } | ||
| case UpperCaseWord: | ||
| w = word.String() | ||
| s := string(word) | ||
| if cfg.allUpper || cfg.screaming { | ||
| w = strings.ToUpper(w) | ||
| for _, r := range s { | ||
| b.WriteRune(unicode.ToUpper(r)) | ||
| } | ||
| } else if cfg.allLower || cfg.whispering { | ||
| w = strings.ToLower(w) | ||
| for _, r := range s { | ||
| b.WriteRune(unicode.ToLower(r)) | ||
| } | ||
| } else if cfg.caseMode == CMAllTitle { | ||
| w = UpperCaseFirst(strings.ToLower(w)) | ||
| first := true | ||
| for _, r := range s { | ||
| if first { | ||
| b.WriteRune(unicode.ToUpper(r)) | ||
| first = false | ||
| } else { | ||
| b.WriteRune(unicode.ToLower(r)) | ||
| } | ||
| } | ||
| } else { | ||
| w = strings.ToLower(w) | ||
| for _, r := range s { | ||
| b.WriteRune(unicode.ToLower(r)) | ||
| } | ||
| } | ||
| case SeparatorWord: | ||
| w = word.String() | ||
| b.WriteString(string(word)) | ||
| default: | ||
| w = word.String() | ||
| b.WriteString(word.String()) | ||
| } | ||
|
|
||
| result = append(result, w) | ||
| } | ||
|
|
||
| delimiter := cfg.delimiter | ||
| if cfg.upperIndicator != "" { | ||
| if cfg.upperIndicator == cfg.delimiter { | ||
| delimiter = cfg.delimiter + cfg.delimiter | ||
| } else { | ||
| delimiter = cfg.upperIndicator | ||
| } | ||
| } | ||
| final := strings.Join(result, delimiter) | ||
| final := b.String() | ||
|
|
||
| if cfg.firstUpper { | ||
| final = UpperCaseFirst(final) | ||
|
|
@@ -352,18 +455,6 @@ func separateOptionsAny(opts []any) ([]any, []any) { | |
| return parseOpts, fmtOpts | ||
| } | ||
|
|
||
| // Helper function to split words in mixed case | ||
| func splitMixCase(input, delimiter string) string { | ||
| var result strings.Builder | ||
| result.Grow(len(input)) | ||
| for i, r := range input { | ||
| if i > 0 && unicode.IsUpper(r) { | ||
| result.WriteString(delimiter) | ||
| } | ||
| result.WriteRune(r) | ||
| } | ||
| return result.String() | ||
| } | ||
|
|
||
| // ToKebabCase converts words into kebab-case format. | ||
| func ToKebabCase(words []Word, opts ...Option) (string, error) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jules word are just
stringkind so we can justlen(string(word))There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, because
Wordis defined as an interface (type Word fmt.Stringer) rather than a concrete type, Go does not allow a direct conversion likestring(word). We would need a type assertion or switch for each underlying type (which was flagged as repetitive in previous review). Adding aLen()method to the interface (or checking for it via type assertion) is the most efficient and idiomatic way to handle this without reflection or repetitive switch cases.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jules so we can type assert it to a string then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot directly type assert to
string(e.g.,word.(string)) becausestringdoes not implement theWordinterface (it lacks theString()method). Furthermore, even if it did, Go treatsSingleCaseWordas a distinct type fromstring, so the assertion would fail. We would have to type assert to the concrete type first (e.g.,word.(SingleCaseWord)) before casting to string, which brings us back to needing a switch statement for every single type. TheLen()interface method avoids this repetition efficiently.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jules what is happening here then: