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

Lab3 #478

Merged
merged 4 commits into from
Jul 24, 2019
Merged

Lab3 #478

merged 4 commits into from
Jul 24, 2019

Conversation

nicko-lee
Copy link
Collaborator

@nicko-lee nicko-lee commented Jun 24, 2019

Fixes #477

Review of colleague's PR #457

Changes proposed in this PR:

  • Add letter sort functionality
  • Add unit tests based on table driven testing

Nicko Lee added 2 commits June 7, 2019 10:35
Implement letters() and sortLetters() and some tests for them. Stil need to fix test cases and understand table driven testing
Get familiar with table driven testing based on the following article
https://dave.cheney.net/2019/05/07/prefer-table-driven-tests
@codecov
Copy link

codecov bot commented Jun 24, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #478   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         108    109    +1     
  Lines        1878   1894   +16     
=====================================
+ Hits         1878   1894   +16
Impacted Files Coverage Δ
03_letters/nickolee/letter_frequency.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 2ac6c58...659ef3d. Read the comment docs.

nicholascross
nicholascross approved these changes Jun 24, 2019
Copy link
Contributor

@nicholascross nicholascross left a comment

Choose a reason for hiding this comment

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

I noticed you could be making use of the assert package to improve the brevity of your unit tests, aside from that it looks good.

@nicholascross nicholascross mentioned this pull request Jun 24, 2019
Copy link
Member

@anzboi anzboi left a comment

Choose a reason for hiding this comment

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

Great work and test coverage.
I would like you to remove all the extra newlines. As for testify, treat it as optional but recommended.

03_letters/nickolee/letter_frequency.go Outdated Show resolved Hide resolved
for _, char := range s {
count[char]++
}

Copy link
Member

Choose a reason for hiding this comment

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

Ditto

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

03_letters/nickolee/letter_frequency.go Show resolved Hide resolved
03_letters/nickolee/letter_frequency.go Outdated Show resolved Hide resolved
// note that Sprintf returns u a formatted string it doesn't print anything

}

Copy link
Member

Choose a reason for hiding this comment

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

Ditto

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Erased from existence

03_letters/nickolee/letter_frequency_test.go Show resolved Hide resolved
03_letters/nickolee/letter_frequency_test.go Show resolved Hide resolved
03_letters/nickolee/letter_frequency_test.go Outdated Show resolved Hide resolved
03_letters/nickolee/letter_frequency_test.go Outdated Show resolved Hide resolved
Mainly around unnecessary whitespace
Copy link
Member

@anzboi anzboi left a comment

Choose a reason for hiding this comment

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

LGTM

@nicko-lee nicko-lee requested a review from samuong as a code owner July 24, 2019 00:58
@nicko-lee nicko-lee merged commit 7761a89 into anz-bank:master Jul 24, 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.

Lab3 - Letter Frequency
3 participants