Diffing_Engine: `Query.CombinedDiff()` shorten lines and consistent null check for `ModifiedObjectDiffernces`

[travispotterBH]

trafficstars

Discovered as part of an issue in another tool. It seems this method can possibly return null for ModifiedObjectDifffernce. 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

[alelom]

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?

[pawelbaran]

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?

https://github.com/BHoM/BHoM_Engine/blob/b0d81439624bf92aa09bad16a7db0ac795294a28/Diffing_Engine/Query/CombinedDiff.cs#L59

[alelom]

I've had a deeper look at the code, and I cannot find any redundant checks. This method does the following:

• individually checks all properties of a first diff object for null
• if they are not null, a concatenation with the properties of a second toAdd 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.

[pawelbaran]

Ok, I looked up the origins of the issue, it is these lines:

https://github.com/BHoM/BHoM_Engine/blob/b0d81439624bf92aa09bad16a7db0ac795294a28/Diffing_Engine/Compute/DiffOneByOne.cs#L96-L97

...modifiedObjectDifferences should be set to a new list to keep consistent.

[FraserGreenroyd]

@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 😉