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

Implemented Lab4 numeronym and test cases #542

Merged
merged 4 commits into from
Jul 15, 2019

Conversation

sirigithub
Copy link
Collaborator

@sirigithub sirigithub commented Jul 7, 2019

Fixes #

Review of colleague's PR #518

Changes proposed in this PR:

  • Implemented executable go program in directory 04_numeronym/sirigithub
  • Implemented a function numeronyms that returns a slice of numeronyms for its input strings
  • Included main method as per spec that calls the above function.
  • Implemented tests for both methods

@codecov
Copy link

codecov bot commented Jul 7, 2019

Codecov Report

Merging #542 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #542   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          95     96    +1     
  Lines        1627   1638   +11     
=====================================
+ Hits         1627   1638   +11
Impacted Files Coverage Δ
04_numeronym/sirigithub/main.go 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f1460e...5483852. Read the comment docs.

n0npax
n0npax previously approved these changes Jul 7, 2019
Copy link
Collaborator

@n0npax n0npax left a comment

Choose a reason for hiding this comment

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

Looks good 👍 The only thing I could suggest is to split iteration over arguments and computing numeronym into separate functions, but it's no required in my mind.

Copy link
Contributor

@camscale camscale left a comment

Choose a reason for hiding this comment

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

LGTM. A good set of test cases too. Just one thing to fix for executing the test case.

04_numeronym/sirigithub/main.go Show resolved Hide resolved
04_numeronym/sirigithub/main_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@camscale camscale left a comment

Choose a reason for hiding this comment

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

LGTM. Great looking Go code. Thanks.

When you merge, can you either do a squash merge, or you could do a local rebase and squash and then force push. This ensures that master does not contain any fixup commits - just to good stuff. It keeps the history clean. If you do a rebase and force-push, ping me on slack when you've done it and i'll re-approve right away (since force pushing drops any current approvals).

@sirigithub sirigithub merged commit c85a563 into anz-bank:master Jul 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants