Skip to content

To compare string ignoring case, use the option StringComparison.OrdinalIgnoreCase #1321

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

To compare string ignoring case, use the option StringComparison.OrdinalIgnoreCase #1321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/core/SIP/SIPEndPoint.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,11 +137,11 @@ public static SIPEndPoint ParseSIPEndPoint(string sipEndPointStr)
return null;
}

if (sipEndPointStr.ToLower().StartsWith("udp:") ||
sipEndPointStr.ToLower().StartsWith("tcp:") ||
sipEndPointStr.ToLower().StartsWith("tls:") ||
sipEndPointStr.ToLower().StartsWith("ws:") ||
sipEndPointStr.ToLower().StartsWith("wss:"))
if (sipEndPointStr.StartsWith("udp:", StringComparison.OrdinalIgnoreCase) ||
sipEndPointStr.StartsWith("tcp:", StringComparison.OrdinalIgnoreCase) ||
sipEndPointStr.StartsWith("tls:", StringComparison.OrdinalIgnoreCase) ||
sipEndPointStr.StartsWith("ws:", StringComparison.OrdinalIgnoreCase) ||
sipEndPointStr.StartsWith("wss:", StringComparison.OrdinalIgnoreCase))
{
return ParseSerialisedSIPEndPoint(sipEndPointStr);
}
Expand Down
18 changes: 8 additions & 10 deletions src/core/SIPEvents/SIPEventPackages.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,16 +163,14 @@ public static bool IsValid(string value)
{
return false;
}
else if (value.ToLower() == "cancelled" || value.ToLower() == "error" || value.ToLower() == "local-bye" ||
value.ToLower() == "rejected" || value.ToLower() == "replaced" || value.ToLower() == "remote-bye" ||
value.ToLower() == "timeout")
{
return true;
}
else
{
return false;
}

return value.Equals("cancelled", StringComparison.OrdinalIgnoreCase) ||
value.Equals("error", StringComparison.OrdinalIgnoreCase) ||
value.Equals("local-bye", StringComparison.OrdinalIgnoreCase) ||
value.Equals("rejected", StringComparison.OrdinalIgnoreCase) ||
value.Equals("replaced", StringComparison.OrdinalIgnoreCase) ||
value.Equals("remote-bye", StringComparison.OrdinalIgnoreCase) ||
value.Equals("timeout", StringComparison.OrdinalIgnoreCase);
Copy link
Member

Choose a reason for hiding this comment

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

Given this is being refactored why not make it even readable:

return value.ToLowerInvariant() is "cancelled" 
                                   or "error"
                                   or "local-bye"
                                   or "rejected"
                                   or "replaced"
                                   or "remote-bye"
                                   or "timeout";

Copy link
Contributor

Choose a reason for hiding this comment

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

That will potentially allocate a string.

The purposed code is a bit more verbose, but potentially more performant.

Unless all input has be previously sanitized and guaranteed to be lower case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couldn't agree more.

Copy link
Member

Choose a reason for hiding this comment

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

I'm struggling to see why the emphasis on such fine grained optimisations (in itself an anti-pattern). This part of the code is dealing with the SIP signalling protocol which has far lower traffic volumes than the media & RTP traffic. And even if you're intent on milking performance from the SIP stack you're starting in the wrong place, for example see the extensive use of regex'es in SIPHeader.cs which will far outweigh any string allocations (not that there have been any performance issues reported in that class the last 20 years).

For 90% of the classes in this library the emphasis should be on readability and maintainability ahead of extracting the last 10% of performance. The RTP stack is an exception.

Unlike this lirbary there is a prototype C# video encoding attempt that could benefit a lot from perfomance improvements.

}

public static SIPEventDialogStateEvent Parse(string value)
Expand Down