Add String.Trim overloads that take a single char#9009
Conversation
karelz
left a comment
karelz
left a comment
There was a problem hiding this comment.
LGTM - 2 minor comments, feel free to ignore them if you disagree.
| { | ||
| for (start = 0; start < Length; start++) | ||
| { | ||
| if (!char.IsWhiteSpace(this[start])) break; |
There was a problem hiding this comment.
Nit: 'break' on a new line? (given you're already cleaning up the code ... not sure if it is a code style rule, maybe it is just my personal preference)
There was a problem hiding this comment.
For what its worth, the coding guidelines allow single line if without braces. Anything else must have braces.
That isn't how I was brought up, but those are the rules :)
There was a problem hiding this comment.
Fixed.
The guidelines say:
A single line statement block can go without braces but the block must be properly indented on its own line and it must not be nested in other statement blocks that use braces
Given that last part and the fact that it is nested in other statement blocks here, I went ahead and moved break to it's own line surrounded by braces.
|
|
||
| return TrimHelper(TrimBoth); | ||
| } | ||
| private string TrimHelper(int trimType) |
There was a problem hiding this comment.
I'd personally change the 3 const ints TrimHead, TrimTrail, TrimBoth, into enum ... given you're already cleaning up and refactoring the code ... just a thought ...
| // Removes a set of characters from the beginning of this string. | ||
| public unsafe string TrimStart(params char[] trimChars) | ||
| { | ||
| if (null == trimChars || trimChars.Length == 0) |
There was a problem hiding this comment.
Nit, I think we try to avoid Yoda conditionals.
|
|
||
| return TrimHelper(TrimBoth); | ||
| } | ||
| private string TrimHelper(int trimType) |
There was a problem hiding this comment.
Might be reasonable to change this method name to something like TrimWhitespaceHelper
danmoseley
left a comment
danmoseley
left a comment
There was a problem hiding this comment.
@stephentoub is it okay to change a public method to unsafe?
Yes, unsafe doesn't actually affect the signature in metadata. |
|
Feedback addressed. Also fixed a confusing/incorrect comment. |
|
@dotnet-bot test this please (failed to restore.. transient?) |
|
@dotnet-bot test Linux ARM Emulator Cross Release Build please (build timeout) |
|
@justinvp Thank you. Could you please port this change to CoreRT when you get a chance? |
|
@jkotas I had troubles with Squash & merge - any tips what you did? |
|
@karelz Just pressed the green button as usual ... nothing special. |
|
Hmmm, I was not brave enough with 'Merge' apparently ... Squash & Merge basically told me "no" |
|
I have done Squash & Merge. |
|
Hmmm, weird. Probably problem between the keyboard and the chair here ... |
* Add String.Trim overloads that take a single char * Cleanup uses of string.Trim/TrimEnd Commit migrated from dotnet/coreclr@fc2448f
8 participants
Sorry for the larger diff -- I decided to cleanup the order and formatting of the Trim methods while I was at it.
Second commit cleans up some uses of Trim/TrimEnd in CoreLib.
https://github.com/dotnet/corefx/issues/14337