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

Add CalcResult<T> class #61

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

Vovanda
Copy link
Member

@Vovanda Vovanda commented Jul 28, 2020

  • Provides the ability to use standard arithmetic operators
  • Replaces the usual approach to throw an exception or return the null in case of an execution failure

@Vovanda Vovanda requested review from Konard and ythosa July 28, 2020 21:51
Vovanda added 2 commits July 29, 2020 21:41
Provides the ability to use standard arithmetic operators
Replaces the usual approach to throw an exception or return the null in case of an execution failure
Add simple integration test
Add tests on arithmetic operations and use operators
Copy link
Member

@ythosa ythosa left a comment

Choose a reason for hiding this comment

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

Круто!!!!! Красавчик!!!! Так держать!!!!!! Любим тебя!!!!!!!!! 🥇 🥇 🥇

Copy link
Member

@Konard Konard left a comment

Choose a reason for hiding this comment

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

@Vovanda what do you think about these issues?

csharp/Platform.Numbers/CalcResult.cs Show resolved Hide resolved
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public CalcResult<T> Add(T a)
{
if (IsValid)
Copy link
Member

Choose a reason for hiding this comment

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

Each if statement will add additional performance overhead. And conditional jump instruction is much slower than addition instruction.

{
_value = Arithmetic<T>.Add(_value, a);
}
catch (Exception e)
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason to add exceptions tracking here?

}
}

public bool IsValid { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Why do you want to store the validity state of the result?

csharp/Platform.Numbers/CalcResult.cs Show resolved Hide resolved
@Konard
Copy link
Member

Konard commented Jul 29, 2020

I suggest you to look into the Integer<T> struct implementation that was removed from this repository. May be we can revive such struct by overloading its operators that will use our dynamically compiled delegates for basic operations. May be we can also rename it to Number<T> (1 letter shorter), but this will also mean we will have to implement support for floating-point numbers (float, double, decimal).

@Konard
Copy link
Member

Konard commented Jul 29, 2020

@Vovanda after this will be finished you can also try to compare our solution with existing one (#31), or may be we can use some ideas from Jon Skeet`s implementation.

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