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 panic version of common operations #48

Closed
wants to merge 10 commits into from

Conversation

dascapytal1559
Copy link

@dascapytal1559 dascapytal1559 commented Jul 17, 2024

I've found these Musts to be very useful in my projects so I thought I'd pitch it.

Feel free to object.

@pmzajaczkowski
Copy link

@govalues Could you please merge it? :)

@eapenkin
Copy link
Collaborator

eapenkin commented Nov 25, 2024

Dear @dascapytal1559,

Thank you for your suggestion! I understand how "Must" methods can be convenient for simplifying code by avoiding explicit error handling. However, the primary goal of this package is to prioritize safety and robustness in decimal computations.

That said, I’d like to share a couple of suggestions that might help achieve similar convenience while keeping computations clear and panic-free:

Function per Formula: Consider breaking down complex formulas into smaller functions. Each function can represent a meaningful part of the computation. This approach not only improves readability but also enhances maintainability by giving clear names to specific steps in the computation.

Favor Composite Methods: Where possible, simplify your formulas algebraically to make use of the composite operations available in the library (e.g., Sub, Prod, AddMul, AddSub, SubMul, SubQuo). These methods are optimized for performance, accuracy, and readability.

I hope these suggestions are helpful!

@dascapytal1559
Copy link
Author

I accept panic-free is a core tenent of this package but I also dont find the suggestions helpful in resolving my pain-point using this package. Thanks for considering anyway, I'll continue to run a fork for my usecases. Feel free to close the PR.

Comment on lines +712 to +718
// UnmarshalJSON implements the json.Unmarshaler interface.
func (d *Decimal) UnmarshalJSON(s []byte) error {
var err error
unquoted := string(s[1 : len(s)-1])
*d, err = Parse(unquoted)
return err
}

Choose a reason for hiding this comment

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

Here you could call Unquote

Suggested change
// UnmarshalJSON implements the json.Unmarshaler interface.
func (d *Decimal) UnmarshalJSON(s []byte) error {
var err error
unquoted := string(s[1 : len(s)-1])
*d, err = Parse(unquoted)
return err
}
// UnmarshalJSON implements the json.Unmarshaler interface.
func (d *Decimal) UnmarshalJSON(s []byte) error {
var err error
unquoted := strings.Unquote(string(s))
*d, err = Parse(unquoted)
return err
}

But also, you could simply do this

Suggested change
// UnmarshalJSON implements the json.Unmarshaler interface.
func (d *Decimal) UnmarshalJSON(s []byte) error {
var err error
unquoted := string(s[1 : len(s)-1])
*d, err = Parse(unquoted)
return err
}
// UnmarshalText implements the [json.UnmarshalerText] interface.
func (d *Decimal) UnmarshalText(s []byte) error {
var err error
*d, err = Parse(string(s))
return err
}

@@ -709,6 +709,29 @@ func (d Decimal) MarshalText() ([]byte, error) {
return []byte(d.String()), nil
}

// UnmarshalJSON implements the json.Unmarshaler interface.

Choose a reason for hiding this comment

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

Suggested change
// UnmarshalJSON implements the json.Unmarshaler interface.
// UnmarshalJSON implements the [json.Unmarshaler] interface.

Comment on lines +720 to +723
// MarshalJSON implements the json.Marshaler interface.
func (d Decimal) MarshalJSON() ([]byte, error) {
return []byte("\"" + d.String() + "\""), nil
}

Choose a reason for hiding this comment

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

Suggested change
// MarshalJSON implements the json.Marshaler interface.
func (d Decimal) MarshalJSON() ([]byte, error) {
return []byte("\"" + d.String() + "\""), nil
}
// MarshalText implements the [json.MarshalerText] interface.
func (d Decimal) MarshalText() ([]byte, error) {
return []byte("\"" + d.String() + "\""), nil
}

Comment on lines +725 to +733
// GobDecode implements the gob.GobDecoder interface for gob serialization.
func (d *Decimal) GobDecode(data []byte) error {
return d.UnmarshalBinary(data)
}

// GobEncode implements the gob.GobEncoder interface for gob serialization.
func (d Decimal) GobEncode() ([]byte, error) {
return d.MarshalBinary()
}

Choose a reason for hiding this comment

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

Suggested change
// GobDecode implements the gob.GobDecoder interface for gob serialization.
func (d *Decimal) GobDecode(data []byte) error {
return d.UnmarshalBinary(data)
}
// GobEncode implements the gob.GobEncoder interface for gob serialization.
func (d Decimal) GobEncode() ([]byte, error) {
return d.MarshalBinary()
}
// GobDecode implements the [gob.GobDecoder] interface for gob serialization.
func (d *Decimal) GobDecode(data []byte) error {
return d.UnmarshalBinary(data)
}
// GobEncode implements the [gob.GobEncoder] interface for gob serialization.
func (d Decimal) GobEncode() ([]byte, error) {
return d.MarshalBinary()
}

@@ -70,6 +88,198 @@ func TestDecimal_Interfaces(t *testing.T) {
}
}

func TestDecimal_UnmarshallJson(t *testing.T) {

Choose a reason for hiding this comment

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

Suggested change
func TestDecimal_UnmarshallJson(t *testing.T) {
func TestDecimal_UnmarshallJSON(t *testing.T) {

}
}

func TestDecimal_MarshallJson(t *testing.T) {

Choose a reason for hiding this comment

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

Suggested change
func TestDecimal_MarshallJson(t *testing.T) {
func TestDecimal_MarshallJSON(t *testing.T) {

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.

4 participants