-
Notifications
You must be signed in to change notification settings - Fork 123
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
Trait Modify Factions #955
base: master
Are you sure you want to change the base?
Conversation
if (traitPrototype.RemoveFactions is null) | ||
return; | ||
|
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.
if (traitPrototype.RemoveFactions is null) | |
return; |
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.
Death for the love of god no. We've had this conversation before.
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.
https://stackoverflow.com/a/6455672
Null lists are pointlessly extra to deal with and probably worse performing, though negligible either way.
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.
Also still wrong, that's from 13 years ago. That predates Dotnet 8 by 12 YEARS.
Null List is the preferred method in Dotnet 8, even according to Microsoft's documentation for Dotnet 8.
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.
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.
Death this link you've posted is relevant to dotnet 4.
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 know
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 found another that better explains what I mean: https://stackoverflow.com/a/1970010
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.
Death this is even more outdated, and irrelevant to what we're doing here. Death the entire reason why the people in StackOverflow(FROM A LITERAL DECADE AGO) are saying not to use Nullable lists is because in older versions of C#, the compiler had no way of finding NullReferences before Runtime, so errors when coding with nullable variables WAS (past tense) difficult to debug.. Nullable List is preferred over Empty List in Dotnet 8 because it's:
- several times faster to guard clause the null conditional vs. entering a foreach with an empty list.
- The dotnet 8 compiler(And IDEs using it) can identify null reference conditions at compile time, you can see this in Intellisense straight up telling you whether variables are potentially null. The compiler will error out if you fail to check a null condition, then tell you the exact line where you failed to check it.
- Build And Test Debug will Test-Fail if you foreach an empty list. Which is why even if you wanted me to do this system over empty lists, I can't, because instead of checking if the list is null, I now have to check if the list is empty. Which is slower and less performant than checking if the list is null.
Here it's being used in the entire TraitSystem because it's the fastest possible way to go through a very wide assortment of lists. It's faster to check which lists are null, than it is to foreach loop each and every list in the prototype to check if they aren't 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.
No, the stackoverflow answers explicitly mention that non-nullable empty lists have worse performance, but are better for maintainability, usability, and readability. That has not changed in a decade.
Co-authored-by: DEATHB4DEFEAT <[email protected]> Signed-off-by: VMSolidus <[email protected]>
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Description
Made by request from Nuclear14, while also adding a trait that was present both in Parkstation, as well as something N14 would like to have. This PR extends the TraitSystem so that it can now also add or remove Factions from a character. It also adds in the Animal Friend trait from Parkstation/Fallout: New Vegas.
Changelog
🆑