Analytical_oM: should `IResult` **not** implement `IComparable`?

[alelom]

Currently, IResult implements IComparable.

This forces all results to implement a CompareTo function.

While this can be useful in several scenarios, it places a mandatory overhead on all implementations of IResult. Further, this overhead will always be additional behavioural logic that the class itself will have to specify, which I believe goes against BHoM principles.

Proposed change

1. Remove IComparable from IResult.
2. Regarding existing implementations, either (please provide feedback here):
   • create a new IComparableResult interface, that implements both IResult and IComparable, and modify existing implementations that leveraged the CompareTo function to implement IComparableResult, OR
   • simply do not touch existing implementations (requires evaluating whether CompareTo was triggered only in the presence of IComparable in any place of the code)
3. On the long term, replace any existing code that leverages CompareTo with suitable Engine methods.

@enarhi @IsakNaslundBh @al-fisher @adecler

[IsakNaslundBh]

The IComparable interface is used to some extent by the structural results to be able to easily sort lists for example. For example list.Sort() require the generic type of the list to be of an IComparable type.

Reason it has been kept in there is for efficiency reasons, as you quite commonly end up with millions of results, and that is where you start to feel the effect of casting as dynamic etc.

Also, would not want a solution that relied on RunExtensionMethod for comparing two results, as that would mean an even larger slowdown.

I agree it goes against the general principles of how we have structured the BHoM, but this was deemed acceptable before for the reasons stated above (being able to use standard list methods and efficiency).

I personally would like to keep it, as I think it makes sense for tabulatable data types such as results that do rely on indexing etc for identification. Can limit it down to only be required on the structural results, but would prefer not to do that to keep some level of generality.

If so, maybe moving the requirement to IAnalyticalResult could be a better fit.

Any reason this has become a problem, or is it more of a general sanity checking (which ofc is good to do!)?

[alelom]

Thanks @IsakNaslundBh for your points.

Any reason this has become a problem, or is it more of a general sanity checking (which ofc is good to do!)?

Why this popped up is because we are doing some works with Results, implementing new ones and reviewing some of the existing classes/interfaces. The CompareTo is placing burden on their implementation that I do not think always make sense and I would like to avoid in certain cases.

I agree it goes against the general principles of how we have structured the BHoM, but this was deemed acceptable before for the reasons stated above (being able to use standard list methods and efficiency).

The usages you mentioned are the kind I had in mind. However, because this goes against BHoM principles, I believe that at least we should have the option of implementing IResults without IComparable. Given what you've said, I think the way to go could be to create an IComparableResult : IResults, IComparable and remove IComparable from IResult. I don't think it's justifiable to impose the CompareTo implementation on every result object (some results may not even be sortable)?

The IComparable interface is used to some extent by the structural results to be able to easily sort lists for example. For example list.Sort() require the generic type of the list to be of an IComparable type.

Reason it has been kept in there is for efficiency reasons, as you quite commonly end up with millions of results, and that is where you start to feel the effect of casting as dynamic etc.

Also, would not want a solution that relied on RunExtensionMethod for comparing two results, as that would mean an even larger slowdown.

Would it not be more likely that we added Engine methods to sort IResult collections rather than couples of results?
In that case, the RunExtensionMethod way would have minimal overhead (reflection invoked only once per Sort). Maybe we can even find more optimised other Reflection patterns to minimise the overhead.

In general I agree that we should not introduce overhead on every single Result, but I think it would be acceptable on the collection as a whole, and it feels like we could find a way to have the Reflection overhead on the collection only. However I don't know how deeply that was explored already, so I might be just wrong there.

[adecler]

A few comments from me from our discussion:

• Given that the CompareTo method needs to be implemented in the oM (lower expertise level users). It is a big ask to expect people to account for all cases (e.g. what happens if the opbject is compared to another type of object ?).
• I would prefer for the IResult to not be IComparable and use something like OrderBy(Func<>) where sorting the results is actually needed. One of the philosophy of the BHoM is to keep code as close as possible to its user cases and this breaks that concept. So would be good to check if using OrderBy(Func<>) is really that much slower.
• If IComparable is needed after all, I agree with @alelom that it would be better to not enforce it on all IResult. I would simply have the relevant objects implement both IResult and IComparable instead of a new interface IComparableResult that doesn't really solve any problem.

[alelom]

Thanks @adecler, agree to all. Only one note:

instead of a new interface IComparableResult that doesn't really solve any problem.

the idea I had behind having IComparableResult was to "officialise" the support of CompareTo for certain categories of results that (will need to demonstrate that they) actually need it. Simply leave the implementation of IComparable as an "opt-in" solution on certain Results is certainly doable, but it feels less like a concerted, agreed framework solution, and more like a patch used in certain cases (which would anyway need to be clearly identified and categorized).

[alelom]

I'm also leaving a note from @IsakNaslundBh during the discussion. The ReadResults adapter method currently leverages the Sorting ability of IComparable results. This simply means that when removing IComparable from base results we will need to align the Adapter too - worth keeping in mind.