Skip to content

Conversation

eriove
Copy link
Contributor

@eriove eriove commented Jan 26, 2016

Operator overloading as discussed in #89 and #113.

We've added the most common operations and the ones we know we will use. It is probably easiest to add more overloads when they are needed.

All code is cross-checked twice by me and @ulflind but I wouldn't be surprised if you find something that needs fixing.

@angularsen
Copy link
Owner

Thanks, I will try to take a look at this soon. A bit occupied with selling my house these days.

@eriove
Copy link
Contributor Author

eriove commented Jan 26, 2016

No problem. Good luck with selling the house.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo, SpeedDividedBy.
Typo, TimeSpanTimesSpeed

@angularsen
Copy link
Owner

What a massive piece of work, great job.
I only found two operator overloads that might be off, everything else looks good to me and with good test coverage. Awesome!

I'm awaiting your feedback on my comments, then let me know when you think it is ready for merge and I'll take a final look at it.

Future improvement

  1. UnitA / UnitA = Ratio ?
  2. UnitA / int|long|double|decimal = UnitA

This one is tricky, should we preserve .NET round behavior so that Length.FromMeters(3) / (int)2 = 1, 1.5 or 2? Internally we use double representations for most units, so it would by probably default to 1.5d internally, and I think that is the most intuitive, but one could argue dividing by integer should round down but I don't think I like that behavior. Rounding up is even less intuitive as only Convert.ToInt32() and Math.Round() methods achieve that.

@eriove
Copy link
Contributor Author

eriove commented Jan 31, 2016

It should be ready to merge now (see comments on your comment, typos should be fixed).

  1. UnitA / UnitA already returns a double which is very intuitive to me
  2. Dividing by a doubleis also implemented, it's quite logical that it behaves as it does right now. I would prefer if the compiler didn't implicitly cast long, int and decimal to double and used the existing overload but the end result makes more sense than the alternatives. Since the returned Unit would always be a double it would be quite strange if it was rounded up or down.

@angularsen
Copy link
Owner

You are right of course, I forgot we already had some operator overloads.
I agree with your reasoning and your latest change looks good. Merging in now.

angularsen added a commit that referenced this pull request Jan 31, 2016
Add operator overloads of the most common unit conversions
@angularsen angularsen merged commit 2c0322e into angularsen:master Jan 31, 2016
@angularsen
Copy link
Owner

Nuget 3.25 out.

@eriove eriove deleted the feature/operator-overloading branch February 28, 2016 08:25
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