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

ObjectListExtensions.OfType should ignore null values #647

Closed
wmrutten opened this issue Jul 25, 2018 · 3 comments · Fixed by FirelyTeam/firely-net-common#156 or #1881
Closed

ObjectListExtensions.OfType should ignore null values #647

wmrutten opened this issue Jul 25, 2018 · 3 comments · Fixed by FirelyTeam/firely-net-common#156 or #1881
Labels
bitesize! breaking change This issue/commit causes a breaking change, and requires a major version upgrade
Milestone

Comments

@wmrutten
Copy link
Contributor

Concerning the extension method ObjectListExtensions.OfType in the Hl7.Fhir.Support library:

  1. Why this custom method, as LINQ also provides a (generic) method OfType<T> with same behavior out of the box?
  2. The LINQ OfType<T> method uses the is operator, which automatically skips/excludes null values but includes subclasses. The custom method compares runtime types, which throws an exception for null values and excludes subclasses.

Suggestions:

  1. Remove this method, use LINQ instead
  2. Or rewrite the method to behave exactly the same as the LINQ method (using is operator)
  3. Or rename this method to e.g. OfExactType (only if the API depends on such behavior)
@ewoutkramer
Copy link
Member

We cannot use the LINQ method here, since it's not the same. OfType in LINQ takes a compile-time specified generic parameter, this method takes a variable type parameter, which is passed at runtime and cannot be used with 'is'.

The behaviour of 'is' can be simulated with Type.IsAssignableFrom(), which is however much slower than direct type comparisons.

I do agree this method is way too specific to be a public extension on IEnumerable, so I will consider inlining this, since it's hardly used.

@wmrutten
Copy link
Contributor Author

wmrutten commented Aug 2, 2018

Agreed, inlining seems appropriate here.

@mmsmits
Copy link
Member

mmsmits commented Aug 12, 2021

Decided to remove ObjectListExtensions entirely + tests. Since this is not used anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bitesize! breaking change This issue/commit causes a breaking change, and requires a major version upgrade
Projects
None yet
4 participants