-
Notifications
You must be signed in to change notification settings - Fork 13
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
minor: string to bytes.Buffer #7
Conversation
Konstantin8105
commented
Aug 26, 2018
- minor optimization "string to bytes.Buffer"
- ignore symbols outside english letters.
@@ -33,24 +33,25 @@ func (h *hacker) Encode(r io.Reader) ([]byte, error) { | |||
} | |||
data := string(d) | |||
data = strings.ToUpper(data) | |||
var encodedValue string | |||
var encode bytes.Buffer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change this variable name to encodedBuffer
or encodedValue
. encode
is a verb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
} | ||
} | ||
|
||
func TestEncodeNotValidLetter(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a valid test case. Can you rename this to TestEncodeValidLetter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, test is valid(Ok). But it is valid test for string with not valid letter(hieroglyph). Could you clarify your opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The letters used in this test is not causing an error, we do not have support of any other language other than english yet, therefore the encode should error out in this scenario but currently it is translating partially without informing the user that part of the data was not translated to morse
Also, could you open an issue for this improvement |
Closing the PR because of inactivity |