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

TEXT-39 WordUtils should use toXxxxCase(int) rather than toXxxxCase(char) #39

Merged
merged 3 commits into from May 9, 2017
Merged

TEXT-39 WordUtils should use toXxxxCase(int) rather than toXxxxCase(char) #39

merged 3 commits into from May 9, 2017

Conversation

ameyjadiye
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented May 5, 2017

Coverage Status

Coverage increased (+0.03%) to 96.653% when pulling 18969d7 on ameyjadiye:TEXT-39 into 7324502 on apache:master.

@ameyjadiye
Copy link
Contributor Author

@chtompki @PascalSchumacher @kinow , please review changes and let me know if more improvements needed here else I have verified everything on my end, all existing test cases are passing with changes.

@kinow
Copy link
Member

kinow commented May 8, 2017

Looks good I think. The code points is handled with the Character#charCount, so it increments with 1 or 2 (when it finds a surrogate pair). I noticed that if I pass a String such as new String(bytes, "UTF-16") the method returns a new String(bytes), ignoring my charset... though I suspect it is smart enough to keep the current charset of the other chars? +1 for merging

Copy link
Member

@chtompki chtompki left a comment

Choose a reason for hiding this comment

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

+1 for merging.

for (int i = 0; i < buffer.length; i++) {
final char ch = buffer[i];
if (isDelimiter(ch, delimiters)) {
for (int index = 0; index < str.length();) {
Copy link
Member

Choose a reason for hiding this comment

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

str.length(); could be replaced with strLen initialized above (just from a performance standpoint). Note, this is extra nit-picky and need not block the PR.

for (int i = 0; i < buffer.length; i++) {
final char ch = buffer[i];
if (isDelimiter(ch, delimiters)) {
for (int index = 0; index < str.length();) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here with str.length();.

final char ch = buffer[i];
if (Character.isUpperCase(ch)) {
buffer[i] = Character.toLowerCase(ch);
for (int index = 0; index < strLen;) {
Copy link
Member

Choose a reason for hiding this comment

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

Right, this is what I was thinking.

* @param delimiters the delimiters
* @return true if it is a delimiter
*/
private static boolean isDelimiter(final int codePoint, final char[] delimiters) {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to not make the signature here:

public static String isDelimiter(final int codePoint, final char... delimiters);

I suppose we could have a debate about making this public v. private, but I'm not sure I see a reason for it to be exclusively private other than not needing to write as much documentation. I'm open to either way here, it was just something that crossed my mind.

Copy link
Contributor Author

@ameyjadiye ameyjadiye May 8, 2017

Choose a reason for hiding this comment

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

Thanks @chtompki for pointing this, I created method first then generated it with eclipse by default it generated private so missed this one, I think this should be public, and there is one more overloaded method with same name also have to be public.

@kinow @PascalSchumacher @britter , thoughts ?

private static boolean isDelimiter(final char ch, final char[] delimiters) 

@chtompki
Copy link
Member

chtompki commented May 8, 2017

Am willing to merge. Just waiting to see what other folks think about what I said here.

@kinow
Copy link
Member

kinow commented May 8, 2017

I'm open to either way here

Same here regarding make that method public :-)

@ameyjadiye
Copy link
Contributor Author

I think most of us are ok with making it public, +1 to merge

@coveralls
Copy link

coveralls commented May 8, 2017

Coverage Status

Coverage increased (+0.03%) to 96.653% when pulling ed55131 on ameyjadiye:TEXT-39 into 7324502 on apache:master.

@coveralls
Copy link

coveralls commented May 8, 2017

Coverage Status

Coverage increased (+0.03%) to 96.653% when pulling d58d014 on ameyjadiye:TEXT-39 into 7324502 on apache:master.

@chtompki
Copy link
Member

chtompki commented May 8, 2017

Looks good. Will pull this in tonight.

@asfgit asfgit merged commit d58d014 into apache:master May 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants