search

ForNeVeR / xaml-math

A collection of .NET libraries for rendering mathematical formulae using the LaTeX typesetting style, for the WPF and Avalonia XAML-based frameworks
MIT License
634 stars 100 forks source link

Refactored to do away with reassignment in `SourceSpan.GetHashCode` #480

Closed Lehonti closed 7 months ago

Lehonti commented 7 months ago

This is an idea for how we could get rid of variable reassignments in the future, making the code more functional (as opposed to procedural)

ForNeVeR commented 7 months ago

Do you really believe that it's worth to change this?

I even have an argument in favor of the previous approach: it makes the code linear, almost as if the type had some linearity (FP-oriented folks tend to love the linear types, are they?).

I mean that you cannot access the previous value of hashcode after each line, so you are sure that you haven't mixed up the step values.

Also, adding a new member in the middle is trivial with the old approach, but will require to change the variable names in the new one.

And it may be my bias, but I find the old approach a bit easier to read. In the new one, I had to eyeball all the lines to check that step1 wasn't reused in calculation of the step3, while it wasn't even possible in the old approach.

So far, I tend to reject this change, though I am open for further discussion if you see other cases when this refactoring would be beneficial.

ygra commented 7 months ago

In this particular case I'd say HashCode.Combine(Length, Source, SourceName) is probably the clearest, although it requires referencing Microsoft.Bcl.HashCode on .NET Framework.