Avoid Trim().Length as empty check and ToLowerInvariant() in string comparison #1567
Conversation
| @@ -365,8 +365,8 @@ internal static bool IsNotRoot(string qualifiedName, out string root) | |||
| /// </returns> | |||
| public static bool BooleanValue(string value) | |||
| { | |||
| string trimmed = value.Trim().ToLowerInvariant(); | |||
| return trimmed.Equals("true") || trimmed.Equals("t"); | |||
| string trimmed = value.Trim(); | |||
There was a problem hiding this comment.
This file has an old ascii corruption, at line 162: fa�ade, for façade, which is a fault by the way, since the comment is not in french. May you please fix that to facade? This thing cause some IDE (Rider) to alter the file at each opening, which is a nuisance, but not worth a dedicated PR.
| @@ -78,7 +78,7 @@ public virtual bool HasOrphanDelete | |||
|
|
|||
| #region Static helper methods | |||
|
|
|||
| private static readonly Dictionary<string, CascadeStyle> Styles = new Dictionary<string, CascadeStyle>(); | |||
| private static readonly Dictionary<string, CascadeStyle> Styles = new Dictionary<string, CascadeStyle>(StringComparer.OrdinalIgnoreCase); | |||
There was a problem hiding this comment.
This looks to me as an undue change. Cascade styles are not case insensitive, even if currently we would not have names conflict by going insensitive.
There was a problem hiding this comment.
Are you sure? Change is based on the following line:
There was a problem hiding this comment.
Well, so the internal usage of this public class is case insensitive. It is then unlikely it could be a breaking change for anyone to render the class case insensitive, but not strictly impossible either.
Well, keep your change in place.
|
There are also a bunch of Update: And in In autoKeyWordsImport = autoKeyWordsImport.ToLowerInvariant();
if (autoKeyWordsImport == Hbm2DDLKeyWords.AutoQuote) |
I didn't touch this |
| public static Dictionary<string, string> HQL_COLLECTION_PROPERTIES; | ||
|
|
||
| private static readonly string COLLECTION_INDEX_LOWER = CollectionPropertyNames.Index.ToLowerInvariant(); | ||
| public static readonly Dictionary<string, string> HQL_COLLECTION_PROPERTIES = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); |
There was a problem hiding this comment.
Usage of this collection looks like just a normal HashSet(). As the class is internal, maybe it's better to replace it.
EDIT: Ok, saw a case where we need dictionary
| /// <summary> | ||
| /// Returns true if content is empty or white space characters only | ||
| /// </summary> | ||
| public bool IsEmptyOrWhitespace() |
There was a problem hiding this comment.
Probably better to align with a String class: make it static; add support for null; call it IsNullOrWhitespace.
There was a problem hiding this comment.
I'm not sure it would give any benefits (at least from dev usability point of view). I tend to check instance methods for emptiness first (and sadly static methods are not even suggested). So I would find this method quicker.
There was a problem hiding this comment.
But let majority rule :) @fredericDelaporte Any preferences?
There was a problem hiding this comment.
Looking at it again, I see a third option: put that into SqlStringHelper. It has already an IsEmpty method (which is actually an IsNullOrEmpty), so it would be more consistent.
About discoverability, this can be improved by making it an extension method.
About aligning with string class, we would then have to move IsEmpty in SqlString too for consistency (and rename it by the way, of course).
There was a problem hiding this comment.
Some instance method is still required as I need to obtain data from GetTrimmedIndexes.
So I think I can add extension method IsNullOrWhitespace to SqlStringHelper that will do null check + call IsEmptyOrWhitespace . Does it make any sense?
There was a problem hiding this comment.
From what I see instance method IsEmptyOrWhitspace needs to exists. So the main question does it improve anything adding additional wrappers over it.
There was a problem hiding this comment.
I am not for having an IsEmptyOrWhitespace on instance along an IsNullOrWhitespace on helper. Better make GetTrimmedIndexes internal.
There was a problem hiding this comment.
Leave it as is.
Simplest option. Agreed.
Comparing SQL keywords should be done with ordinal comparisons. But invariant comparisons are kept for object names, because at least with SQL Server these comparisons are collation sensitive. (So ideally object names comparisons should even have their culture configurable...) An undue current culture comparison for SqlString.EndsWithCaseInsensitive is also fixed. Follow-up to nhibernate#1567.
Comparing SQL keywords should be done with ordinal comparisons. But invariant comparisons are kept for object names, because at least with SQL Server these comparisons are collation sensitive. (So ideally object names comparisons should even have their culture configurable...) An undue current culture comparison for SqlString.EndsWithCaseInsensitive is also fixed. Follow-up to #1567.
And made another round of setting ordinal string comparison