-
Notifications
You must be signed in to change notification settings - Fork 487
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
Enhance "look" ability #162
Conversation
Codecov Report
@@ Coverage Diff @@
## master #162 +/- ##
==========================================
+ Coverage 92.32% 92.35% +0.03%
==========================================
Files 83 83
Lines 1055 1060 +5
Branches 173 177 +4
==========================================
+ Hits 974 979 +5
Misses 66 66
Partials 15 15
Continue to review full report at Codecov.
|
@pigalot I added a |
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.
Hey @pigalot, nice work! Just a couple of comments, please review.
.map(offset => unit.getSensedSpaceAt(direction, offset)) | ||
.some(space => { | ||
sensedSpaces.push(space); | ||
return space && space.isWall && space.isWall(); |
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.
We can safely assume any space returned by unit.getSensedSpaceAt()
will have the isWall
method so this check should be simplified.
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.
That would be true but the fakes given by the existing unit tests are not of type Space
so do not have .isWall
method.
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.
Right, you should add isWall()
to the mocks though now that a simple string does not suffice.
const space1 = { isWall: () => false };
const space2 = { isWall: () => false };
...
unit.getSensedSpaceAt
.mockReturnValueOnce(space1)
.mockReturnValueOnce(space2)
...
const sensedSpaces = []; | ||
offsets | ||
.map(offset => unit.getSensedSpaceAt(direction, offset)) | ||
.some(space => { |
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.
Have you thought about using spaces.slice()
instead of .some()
and an auxiliary array? I think it's cleaner than .some()
.
firstWallIndex
can be calculated with spaces.findIndex()
. If a wall is found, then slice the spaces array up to that wall. If not, return all spaces. What do you think?
Fixes #160