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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

Lehonti
Copy link
Contributor

@Lehonti Lehonti commented Dec 15, 2023

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)

This is an idea for how we could get rid of variable reassignments in the future
@Lehonti Lehonti changed the title Refactored to avoid do away reassignment in SourceSpan.GetHashCode Refactored to do away reassignment in SourceSpan.GetHashCode Dec 15, 2023
@Lehonti Lehonti changed the title Refactored to do away reassignment in SourceSpan.GetHashCode Refactored to do away with reassignment in SourceSpan.GetHashCode Dec 15, 2023
@ForNeVeR
Copy link
Owner

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.

@ForNeVeR ForNeVeR closed this Dec 15, 2023
@ygra
Copy link
Contributor

ygra commented Dec 19, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants