Open travispotterBH opened 1 year ago
This method was last modified by @pawelbaran adding additional ternaries. I can't recall why he needed it to be that way exactly -- @pawelbaran can you comment on this issue?
Sure, the original issue was about the returned object carrying Linq
expressions that were not evaluated ahead of returning from the method. This is why we have added .ToList()
in https://github.com/BHoM/BHoM_Engine/commit/ef5dcef925d7de33ddfe976d69b139ae20f79f5c to trigger the evaluation.
However, this issue seems to originate from somewhere else: as can be seen in the line below, CombinedDiff
will only return ModifiedObjectsDifferences
as null
if it was null
on the input (at least from how I read the code). So the null
value needs to be assigned to the diff on some other step - maybe on creation of the original object?
I've had a deeper look at the code, and I cannot find any redundant checks. This method does the following:
diff
object for nulltoAdd
diff object is attempted. The properties of the toAdd
diff object are also checked for null before the concatenation completes.Thanks for commenting @pawelbaran. Actually I can't figure out how one can get the ModifiedObjectDifferences
as null
. The entire line is:
https://github.com/BHoM/BHoM_Engine/blob/b0d81439624bf92aa09bad16a7db0ac795294a28/Diffing_Engine/Query/CombinedDiff.cs#L59-L60
so if ModifiedObjectDifferences
was null on the first diff
object, this condition will kick in:
: toAdd.ModifiedObjectsDifferences ?? new List<ObjectDifferences>(),
resulting in either the toAdd.ModifiedObjectsDifferences
as a result (if it is not null
), or in an empty List<ObjectDifferences>
.
Are you able to provide a replication script or unit test @travispotterBH ?
By the look of things all seems in order - I can't find a way to get null properties out of a Diff here. The only way to get something null out is by specifying two null
Diff inputs; however, a non-null output Diff will never have any of its properties null, if I'm not mistaken.
I can only spot a single redundant null check here: https://github.com/BHoM/BHoM_Engine/blob/b0d81439624bf92aa09bad16a7db0ac795294a28/Diffing_Engine/Query/CombinedDiff.cs#L58
where we could replace the ?.
accessor with a .
at the start of the line, as we check for diff
not null already on line 48.
Ok, I looked up the origins of the issue, it is these lines:
...modifiedObjectDifferences
should be set to a new list to keep consistent.
@pawelbaran has hit the line on the head there - should be set to a new list.
Also, the length of the line highlighted by @travispotterBH is ridiculous - it's a lot of nested ternaries which should really be broken out to make the code more readable for the average engineer working within the BHoM eco system 😉
Discovered as part of an issue in another tool. It seems this method can possibly return
null
forModifiedObjectDifffernce.
It is difficult to understand what is going on in this method due to the ternaries, and it appears to have multiple checks for null:!= null
,??
, etc.https://github.com/BHoM/BHoM_Engine/blob/ef5dcef925d7de33ddfe976d69b139ae20f79f5c/Diffing_Engine/Query/CombinedDiff.cs#L54-L63