Nullable denominator review

[lipchev]

Ok, I'm now pointing at the right branch:

• the two additions are fairly straightforward

What do you think about making default(Fraction).State == FractionState.IsNormalized?

[danm-de]

What do you think about the making default(Fraction).State == FractionState.IsNormalized?

We could declare the status enum as a backing field again and work with a nullable field there (just like with the denominator).

[lipchev]

Oh, no need- I think we can either

• switch the order of the the enum states (making IsNormalized the first one)
• keep the enum as it is, but have a boolean backing field (defaulting to normalized) that we use to return the desired FractionState in the property getter

[danm-de]

Oh, no need- I think we can

• switch the order of the the enum states (making IsNormalized the first one)
• keep the enum as it is, but have a boolean backing field (defaulting to true) that we use to return the desired FractionState in the property getter

Is there another reason why the FractionState needs to be queried? Would an IsImproper property also suffice?

If so, I would prefer your suggestion with a BackingField (bool _isImproper “?). The State property remains only for compatibility reasons and is marked obsolete. What do you think?

[lipchev]

Oh, no need- I think we can

• switch the order of the the enum states (making IsNormalized the first one)
• keep the enum as it is, but have a boolean backing field (defaulting to true) that we use to return the desired FractionState in the property getter

Is there another reason why the FractionState needs to be queried? Would an IsImproper property also suffice?

If so, I would prefer your suggestion with a BackingField (bool _isImproper “?). The State property remains only for compatibility reasons and is marked obsolete. What do you think?

This is my preferred option, I'm just just struggling to find a good name for the field/property. new Fraction(1, 3, false) isn't improper it just hasn't been normalized explicitly . For the property I'm thinking IsNormalized. The field is tricky - here are some suggestions from the AI:

• _notExplicitlyNormalized
• _normalizationNotApplied
• _normalizationNotForced
• _normalizationNotSpecified
• _awaitingNormalization
• _normalizationDeferred
• _normalizationPending
• _normalizationNotEnforced

[danm-de]

Unfortunately, the constructor parameter probably needs to be renamed.

var a = new Fraction(1, 3, reduce: false);
var b = new Fraction(1, 3, reduce: true);

As backing-field: _unconfirmedNormalization?
default(Fraction) results in _unconfirmedNormalization = false

We leave out the IsNormalized property entirely - the benefit is small (if any).

[lipchev]

Unfortunately, the constructor parameter probably needs to be renamed.

var a = new Fraction(1, 3, reduce: false);
var b = new Fraction(1, 3, reduce: true);

As backing-field: _unconfirmedNormalization? default(Fraction) results in _unconfirmedNormalization = false

We leave out the IsNormalized property entirely - the benefit is small (if any).

unconfirmed suggests to me that we're awaiting confirmation.. ("where is the Confirm(..) method?")
Here's my order of preference:

1. _normalizationDeferred
2. _normalizationPending
3. _normalizationNotApplied

[danm-de]

_normalizationNotApplied sounds good to me.

The name of the constructor parameter can be either normalize or reduce. Come to think of it - "normalize" is an explicit instruction - so not a bad name so far.

[lipchev]

_normalizationNotApplied sounds good to me.

The name of the constructor parameter can be either normalize or reduce. Come to think of it - "normalize" is an explicit instruction - so not a bad name so far.

As a verb, normalize is better, although (if starting clean) I'd personally make anything that's not an implicit constructor, or a variation of the standard tuple (BigInteger, BigInteger) constructor into a factory method. Especially the constructor from double!

[danm-de]

I agree with you. Not all of the design decisions back then were good. However, I would not like to remove these constructors in order to keep the breaking changes within an acceptable range.

[lipchev]

Sure, I was thinking of adding some additional comments to all public members but didn't want to pollute the diffs before (we should have done the solution-reformatting stuff earlier...).
Should I make another PR or are you working on it?

[danm-de]

The only reason I duplicated your branch in a second pull request was because I lost track of all the changes 🥲 😄 .

I am currently in the clinic (my employer) and am busy with other topics today.