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

Suggested fix for NullReferenceException in C# #140

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dafbyte
Copy link

@dafbyte dafbyte commented Mar 21, 2023

This is a suggested fix for issues #138 & #139

@dmsnell
Copy link

dmsnell commented Mar 21, 2023

Is there a use-case for passing null into these functions? wouldn't it be valid to require real strings?

Diff diff = new Diff(Operation.EQUAL, null ?? '');
string actualString = diff.ToString();

@dafbyte
Copy link
Author

dafbyte commented Mar 21, 2023

No, there is no proper use-case for it.

However, please keep in mind that this code is proof of concept for testing, and a user might accidentally pass a string variable containing null.

@dmsnell
Copy link

dmsnell commented Mar 21, 2023

throwing inside the class constructor could make the invalid value more immediately obvious. I wonder how far you can get with a null input before realizing the data is corrupted, if we embed these safeties inside of particular class methods.

@dafbyte
Copy link
Author

dafbyte commented Mar 21, 2023

Sounds good.
What would you think about defaulting the value to an empty string?

Technically this use-case is not by design, so this kind of user code is a grey area.

@dmsnell
Copy link

dmsnell commented Mar 21, 2023

this is a random personal opinion, but I prefer dealing with these errors as soon as possible. a default value seems more reasonable to me than defaulting deep inside the class then because it demarcates the good and bad input right where it comes in (vs. somewhere inside where we have to hope the other methods also make this check)

not sure how the C# folks prefer to handle exceptions. if it's more normative like Python I think there's a case to be made about simply asserting and throwing, but if it's more like JavaScript then a default seems more appropriate.

caveat: I don't have commit access here, but I do maintain #80 where I fixed some Unicode issues and that's what we use in our apps, given that there's mostly silence from the maintainer.

@dmsnell
Copy link

dmsnell commented Mar 21, 2023

Oops! I'm sure you saw this already, but it looks like an extra space was introduced at the beginning of every line in there…

@dafbyte
Copy link
Author

dafbyte commented Mar 21, 2023

Yap, I've noticed it after commit (CRLF settings).
I've fixed the commit to check the values in Ctor.
This is consistent to other implementations as you specified.

@dmsnell
Copy link

dmsnell commented Mar 21, 2023

let's see if @NeilFraser wants to chime in

@RenderMichael
Copy link

The corresponding exception here is ArgumentNullException. NullReferenceExceptions should never be thrown by user code and is always considered a bug

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants