Skip to content
This repository has been archived by the owner on Jun 15, 2021. It is now read-only.

Fix issue with words length #5

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

Fix issue with words length #5

wants to merge 1 commit into from

Conversation

pamelin
Copy link
Contributor

@pamelin pamelin commented Nov 13, 2018

No description provided.

@pamelin pamelin requested a review from a team November 13, 2018 17:36
jollinshead
jollinshead suggested changes Nov 14, 2018
View reviewed changes
Copy link
Contributor

@jollinshead jollinshead left a comment

Choose a reason for hiding this comment

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

As the value 4 is used in multiple locations, could this be moved to a const and referenced where used?

@pamelin
Copy link
Contributor Author

pamelin commented Nov 14, 2018 •
edited
Loading

@jollinshead semantically they are different things. One is the length of the words slice another is the index of the message in the words slice, which happens to be the same number

@jollinshead
Copy link
Contributor

jollinshead commented Nov 14, 2018 •
edited
Loading

@pamelin But your condition (if len(words) > 4 {) is in place to ensure that the slice is large enough to include an index of words[4], so the 4 used in the condition is dictated by the index.

@jollinshead
Copy link
Contributor

@pamelin I can approve this fix. But could you raise an issue to refactor this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jollinshead jollinshead jollinshead approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pamelin @jollinshead