Refactoring of contracts/evoting/types/ballots_test.go : #184
Conversation
defined constants for the most duplicated strings (select, rank, text and the string used when an answer could not be marshaled) to reduce code smells.
Pull Request Test Coverage Report for Build 3260389972Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
| const SELECT_STR = "select:" | ||
| const RANK_STR = "rank:" | ||
| const TEXT_STR = "text:" | ||
| const UNMARSHALING_RANK_STR = "could not unmarshal rank answers: " | ||
| const UNMARSHALING_TEXT_STR = "could not unmarshal text answers: " |
There was a problem hiding this comment.
We use camel case for constants as well. You can also declare them altogether since they're related, see PR #183 or contract/evoting.go for examples
There was a problem hiding this comment.
yes, I think it would make sense and we will be able to use them in some functions in ballots.go such as unmarshal(...) or MaxEncodedSize(...). The only problem I can see is that the format is slightly different between the strings in the tests and the ones in ballots.go and that they are mostly recurrent in the tests.
Thanks for the note on the camel case convention, I have to get rid of my Java habits :)
There was a problem hiding this comment.
Do you mean because of the : ? Since this is just formatting it does not have to be part of the string. It could be written as a join in the tests.
This might be overthinking it (although quite clean), but we could use the Question interface to force the Text/Rank/Select questions to re-define the "String()" function which would simply return "select"/"rank"/"text" and could be called everywhere. It would avoid redefining or hardcoding those strings everywhere. It's just an idea though ;).
Added a specific string related to each kind of question to avoid hardcoding them and a getter for these strings in the Interface Question + corrected constants in the tests
contracts/evoting/types/ballots.go
Outdated
| @@ -401,6 +407,7 @@ type Question interface { | |||
| GetMaxN() uint | |||
| GetMinN() uint | |||
| GetChoicesLength() int | |||
| GetString() string | |||
There was a problem hiding this comment.
In this case GetID() string feels more appropriate.
There was a problem hiding this comment.
Then, you can rename selectStr to selectID, same for rank and text.
There was a problem hiding this comment.
Ok, same thing for the constants in the test file, right ?
|
Kudos, SonarCloud Quality Gate passed! |
Defined constants for the most duplicated strings (select, rank, text and the string used when an answer could not be marshaled) to reduce code smells.