-
Notifications
You must be signed in to change notification settings - Fork 179
Added Null Checks to Util.cs (issue 403) #412
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: Development
Are you sure you want to change the base?
Conversation
Added Null Checks (LogExperts#403)
Thanks for your contribution, but be aware and double check again please, if you throw the exception, the calling function might not handle the exception, this will lead to unexpected crashes. Please check if all the thrown exception are handled correctly in the calling functions and if not try to implement it, but not just with a "try-catch" |
@Hirogen all the exception throwing in the commit are valid as they do exactly the same as before. The only difference now is that it is explicitly in the code and in case of a debug it is easier to see the culprit. |
that is true, the problem I have is, it changes nothing really, its the status quo, NULL shouldn't even be possible in this situations and if it is, then something other is wrong, and this should be avoided in the first place. The only thing this change does, is remove the warnings, but it will not fix the underlying problem, that there could be null references at a place where null references should not be. So it will only move the goal post to another position, but not fix anything. NULL, is always the worst case scenario and should be avoided at all times. |
@B0risev1ch please add todos, that the null values need to be checked at the calling position of the function, null values should not be happen |
Thank you to the project maintainers and contributors for your feedback — and apologies for the delay in addressing this commit. This commit proposes the following updates:
Returns int.MaxValue when the source is null/empty (i.e., there's nothing to search in), Returns 0 when the search term is null/empty (i.e., logically matches everything).
Null checks for text, g, and font are marked with a // TODO: since this method is currently unused in the project. These changes aim to improve robustness and readability while aligning with best practices around null safety (I hope). Let me know if any further adjustments are needed. |
{ | ||
ArgumentNullException.ThrowIfNull(s1); | ||
ArgumentNullException.ThrowIfNull(s2); | ||
s1 ??= string.Empty; |
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.
I am not sure this is a correct way to handle s1 and s2 in case of null. You are hiding the fact that there is a null there and I think that in case of null you should not even call this method.
In short:
I would like to know in case we got here with null and prevent it in the future.
{ | ||
i = fileName.LastIndexOf('/'); | ||
} | ||
public static string GetNameFromPath (string fileName) => fileName is null ? string.Empty : Path.GetFileName(fileName); |
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.
Why not throw an exception in case fileName is null? Why hide it?
|
||
return fileName[(i + 1)..]; | ||
} | ||
public static string StripExtension (string fileName) => fileName is null ? string.Empty : Path.GetFileNameWithoutExtension(fileName); |
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.
Why not throw an exception in case fileName is null? Why hide it?
{ | ||
i = fileName.Length - 1; | ||
} | ||
public static string GetExtension (string fileName) => fileName is null ? string.Empty : Path.GetExtension(fileName).TrimStart('.'); |
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.
Why not throw an exception in case fileName is null? Why hide it?
ArgumentNullException.ThrowIfNull(src); | ||
ArgumentNullException.ThrowIfNull(dest); | ||
|
||
if (dest is null || dest.Length == 0) |
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.
Why to hide? so we will know how to prevent this in the future
Added Null Checks (#403)