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 golang version of emojihash and colorhash #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

osmaczko
Copy link

No description provided.

Copy link

@KilianKae KilianKae left a comment

Choose a reason for hiding this comment

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

Hey! I started the review on the emojihash code, I will probably need another round.
I did not look at the rest of the code.
Could you add a few more comments? e.g. what does BigBase exactly do?

@@ -4,3 +4,11 @@ secure emojihash map for hex strings
$ python3 main.py B8B92Cc1Fbe8E425184769B296BAD43245Ad2C84
> 🌙🔕🏒🍝🥁🏟🐽😳🌲🏈🍁🕘👶😠🧸💙
```

go-lang version of emojihash and colorhash (output as sequence of color indexes 0-31)

Choose a reason for hiding this comment

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

Any reasons for go-lang not just go. Here and in folder structure?


import "math/big"

func ToBigBase(value *big.Int, base uint64) (res [](uint64)) {

Choose a reason for hiding this comment

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

You don't need the () around the uint64 in the return type.

return
}

func toBigBaseImpl(value *big.Int, base uint64, res *[](uint64)) {

Choose a reason for hiding this comment

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

Same as above

} else if hashLen > len(indexes) {
prependLen := hashLen - len(indexes)
for i := 0; i < prependLen; i++ {
indexes = append([](uint64){0}, indexes...)

Choose a reason for hiding this comment

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

No need for () around uint64

"strings"
)

func ToEmojiHash(value *big.Int, hashLen int, alphabet *[]string) (hash []string, err error) {

Choose a reason for hiding this comment

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

err is used nowhere. Just (hash []string, error) is enough

Choose a reason for hiding this comment

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

I would suggest to not use a pointer to the alphabet slice.
-> alphabet []string


func ToEmojiHash(value *big.Int, hashLen int, alphabet *[]string) (hash []string, err error) {
valueBitLen := value.BitLen()
alphabetLen := new(big.Int).SetInt64(int64(len(*alphabet)))

Choose a reason for hiding this comment

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

I would suggest alphabetLen := uint64(len(*alphabet)) to keep it readable.
Simplifies line 15.
Adds a new(big.Int).SetUint64(alphabetLen) to line 26

return hash, nil
}

func LoadEmojisAlphabet() (*[]string, error) {

Choose a reason for hiding this comment

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

I would suggest not to return a pointer to the alphabet.

return nil, err
}

alphabet := make([]string, 0)

Choose a reason for hiding this comment

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

Instantiating the slice like this var alphabet []string would be the more common approach.

return
}

func toBigBaseImpl(value *big.Int, base uint64, res *[](uint64)) {

Choose a reason for hiding this comment

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

To be more in line with the standard libs I would suggest an approach in which the slice is returned and not referenced to. Does anything speak against that?

0kok0 pushed a commit that referenced this pull request Jun 22, 2022
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.

2 participants