haze518 commented on code in PR #1995:
URL: https://github.com/apache/iggy/pull/1995#discussion_r2200425737
##########
foreign/go/contracts/identifier.go:
##########
@@ -30,22 +32,45 @@ const (
StringId IdKind = 2
)
-func NewIdentifier(id any) Identifier {
- var kind IdKind
- var length int
-
- switch v := id.(type) {
- case int:
- kind = NumericId
- length = 4
- case string:
- kind = StringId
- length = len(v)
+// NewNumericIdentifier creates a new identifier from the given numeric value.
+func NewNumericIdentifier(value uint32) (Identifier, error) {
+ if value == 0 {
+ return Identifier{}, ierror.InvalidIdentifier
}
+ return Identifier{
+ Kind: NumericId,
+ Length: 4,
+ Value: value,
+ }, nil
+}
+// NewStringIdentifier creates a new identifier from the given string value.
+func NewStringIdentifier(value string) (Identifier, error) {
+ length := len(value)
+ if length == 0 || length > 255 {
+ return Identifier{}, ierror.InvalidIdentifier
+ }
return Identifier{
- Kind: kind,
- Length: length,
- Value: id,
+ Kind: StringId,
+ Length: len(value),
+ Value: value,
+ }, nil
+}
+
+// GetU32Value returns the numeric value of the identifier.
+func (id Identifier) GetU32Value() (uint32, error) {
+ if id.Kind != NumericId || id.Length != 4 {
+ return 0, ierror.ResourceNotFound
}
+
+ return id.Value.(uint32), nil
Review Comment:
I don't really like that value is of type any, because there’s a risk of
panic in this place if someone constructs an Identifier incorrectly
##########
foreign/go/contracts/identifier.go:
##########
@@ -30,22 +32,45 @@ const (
StringId IdKind = 2
)
-func NewIdentifier(id any) Identifier {
- var kind IdKind
- var length int
-
- switch v := id.(type) {
- case int:
- kind = NumericId
- length = 4
- case string:
- kind = StringId
- length = len(v)
+// NewNumericIdentifier creates a new identifier from the given numeric value.
+func NewNumericIdentifier(value uint32) (Identifier, error) {
+ if value == 0 {
+ return Identifier{}, ierror.InvalidIdentifier
}
+ return Identifier{
+ Kind: NumericId,
+ Length: 4,
+ Value: value,
+ }, nil
+}
+// NewStringIdentifier creates a new identifier from the given string value.
+func NewStringIdentifier(value string) (Identifier, error) {
+ length := len(value)
+ if length == 0 || length > 255 {
+ return Identifier{}, ierror.InvalidIdentifier
+ }
return Identifier{
- Kind: kind,
- Length: length,
- Value: id,
+ Kind: StringId,
Review Comment:
By the way, what do you think - maybe it makes sense to switch the enum
definition to use iota, since that’s the most common way to define such
constants?
```go
type IdKind int
const (
NumericId IdKind = iota
StringId
)
```
##########
foreign/go/contracts/identifier.go:
##########
@@ -30,22 +32,45 @@ const (
StringId IdKind = 2
)
-func NewIdentifier(id any) Identifier {
- var kind IdKind
- var length int
-
- switch v := id.(type) {
- case int:
- kind = NumericId
- length = 4
- case string:
- kind = StringId
- length = len(v)
+// NewNumericIdentifier creates a new identifier from the given numeric value.
+func NewNumericIdentifier(value uint32) (Identifier, error) {
+ if value == 0 {
+ return Identifier{}, ierror.InvalidIdentifier
}
+ return Identifier{
+ Kind: NumericId,
+ Length: 4,
+ Value: value,
+ }, nil
+}
+// NewStringIdentifier creates a new identifier from the given string value.
+func NewStringIdentifier(value string) (Identifier, error) {
+ length := len(value)
+ if length == 0 || length > 255 {
+ return Identifier{}, ierror.InvalidIdentifier
+ }
return Identifier{
- Kind: kind,
- Length: length,
- Value: id,
+ Kind: StringId,
+ Length: len(value),
+ Value: value,
+ }, nil
+}
+
+// GetU32Value returns the numeric value of the identifier.
+func (id Identifier) GetU32Value() (uint32, error) {
+ if id.Kind != NumericId || id.Length != 4 {
+ return 0, ierror.ResourceNotFound
}
+
+ return id.Value.(uint32), nil
Review Comment:
What do you think about changing the value field from any to []byte, just
like it’s currently done in rs
##########
foreign/go/contracts/identifier.go:
##########
@@ -30,22 +32,45 @@ const (
StringId IdKind = 2
)
-func NewIdentifier(id any) Identifier {
- var kind IdKind
- var length int
-
- switch v := id.(type) {
- case int:
- kind = NumericId
- length = 4
- case string:
- kind = StringId
- length = len(v)
+// NewNumericIdentifier creates a new identifier from the given numeric value.
+func NewNumericIdentifier(value uint32) (Identifier, error) {
+ if value == 0 {
+ return Identifier{}, ierror.InvalidIdentifier
}
+ return Identifier{
+ Kind: NumericId,
+ Length: 4,
+ Value: value,
+ }, nil
+}
+// NewStringIdentifier creates a new identifier from the given string value.
+func NewStringIdentifier(value string) (Identifier, error) {
+ length := len(value)
+ if length == 0 || length > 255 {
+ return Identifier{}, ierror.InvalidIdentifier
+ }
return Identifier{
- Kind: kind,
- Length: length,
- Value: id,
+ Kind: StringId,
+ Length: len(value),
+ Value: value,
+ }, nil
+}
+
+// GetU32Value returns the numeric value of the identifier.
+func (id Identifier) GetU32Value() (uint32, error) {
+ if id.Kind != NumericId || id.Length != 4 {
+ return 0, ierror.ResourceNotFound
}
+
+ return id.Value.(uint32), nil
+}
+
+// GetStringValue returns the string value of the identifier.
+func (id Identifier) GetStringValue() (string, error) {
+ if id.Kind != StringId {
+ return "", ierror.InvalidIdentifier
+ }
+
+ return id.Value.(string), nil
Review Comment:
and here via string(id.Value)
##########
foreign/go/contracts/identifier.go:
##########
@@ -30,22 +32,45 @@ const (
StringId IdKind = 2
)
-func NewIdentifier(id any) Identifier {
- var kind IdKind
- var length int
-
- switch v := id.(type) {
- case int:
- kind = NumericId
- length = 4
- case string:
- kind = StringId
- length = len(v)
+// NewNumericIdentifier creates a new identifier from the given numeric value.
+func NewNumericIdentifier(value uint32) (Identifier, error) {
Review Comment:
It seems that the init function could be implemented using generics, what do
you think?
For example, something like this (and if needed, the logic can be moved to
helper functions):
```go
type IdentifierInput interface {
~uint32 | ~string
}
func NewIdentifier[T IdentifierInput](val T) (Identifier, error) {
switch any(val).(type) {
case uint32:
if val.(uint32) == 0 {
return Identifier{}, ErrInvalidIdentifier
}
num := val.(uint32)
bytes := make([]byte, 4)
binary.LittleEndian.PutUint32(bytes, num)
return Identifier{
Kind: NumericId,
Length: 4,
Value: bytes,
}, nil
case string:
s := val.(string)
n := len(s)
if n == 0 || n > 255 {
return Identifier{}, ErrInvalidIdentifier
}
return Identifier{
Kind: StringId,
Length: n,
Value: []byte(s),
}, nil
default:
return Identifier{}, ErrInvalidIdentifier
}
}
```
##########
foreign/go/contracts/identifier.go:
##########
@@ -30,22 +32,45 @@ const (
StringId IdKind = 2
)
-func NewIdentifier(id any) Identifier {
- var kind IdKind
- var length int
-
- switch v := id.(type) {
- case int:
- kind = NumericId
- length = 4
- case string:
- kind = StringId
- length = len(v)
+// NewNumericIdentifier creates a new identifier from the given numeric value.
+func NewNumericIdentifier(value uint32) (Identifier, error) {
+ if value == 0 {
+ return Identifier{}, ierror.InvalidIdentifier
}
+ return Identifier{
+ Kind: NumericId,
+ Length: 4,
+ Value: value,
+ }, nil
+}
+// NewStringIdentifier creates a new identifier from the given string value.
+func NewStringIdentifier(value string) (Identifier, error) {
+ length := len(value)
+ if length == 0 || length > 255 {
+ return Identifier{}, ierror.InvalidIdentifier
+ }
return Identifier{
- Kind: kind,
- Length: length,
- Value: id,
+ Kind: StringId,
+ Length: len(value),
+ Value: value,
+ }, nil
+}
+
+// GetU32Value returns the numeric value of the identifier.
+func (id Identifier) GetU32Value() (uint32, error) {
+ if id.Kind != NumericId || id.Length != 4 {
+ return 0, ierror.ResourceNotFound
}
+
+ return id.Value.(uint32), nil
Review Comment:
In that case, we could safely extract the value like this:
binary.LittleEndian.Uint32(id.Value)
##########
foreign/go/contracts/identifier.go:
##########
@@ -30,22 +32,45 @@ const (
StringId IdKind = 2
)
-func NewIdentifier(id any) Identifier {
- var kind IdKind
- var length int
-
- switch v := id.(type) {
- case int:
- kind = NumericId
- length = 4
- case string:
- kind = StringId
- length = len(v)
+// NewNumericIdentifier creates a new identifier from the given numeric value.
+func NewNumericIdentifier(value uint32) (Identifier, error) {
+ if value == 0 {
+ return Identifier{}, ierror.InvalidIdentifier
}
+ return Identifier{
+ Kind: NumericId,
+ Length: 4,
+ Value: value,
+ }, nil
+}
+// NewStringIdentifier creates a new identifier from the given string value.
+func NewStringIdentifier(value string) (Identifier, error) {
+ length := len(value)
+ if length == 0 || length > 255 {
+ return Identifier{}, ierror.InvalidIdentifier
+ }
return Identifier{
- Kind: kind,
- Length: length,
- Value: id,
+ Kind: StringId,
+ Length: len(value),
+ Value: value,
+ }, nil
+}
+
+// GetU32Value returns the numeric value of the identifier.
+func (id Identifier) GetU32Value() (uint32, error) {
Review Comment:
In go, such methods are usually implemented without the get prefix. You can
simply name them Uint32 or StringValue
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
