Skip to content

Series example refactoring#727

Merged
FridaTveit merged 3 commits into
exercism:masterfrom
ilya-khadykin:Series_example_refactoring
Aug 5, 2017
Merged

Series example refactoring#727
FridaTveit merged 3 commits into
exercism:masterfrom
ilya-khadykin:Series_example_refactoring

Conversation

@ilya-khadykin
Copy link
Copy Markdown
Contributor

This resolves the following issue: series: simplify example implementation

All tests passed


Reviewer Resources:

Track Policies

Copy link
Copy Markdown
Contributor

@FridaTveit FridaTveit left a comment

Choose a reason for hiding this comment

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

This looks great, thanks @m-a-ge! :) I've put one comment but it's a very minor thing so feel free to ignore it if you disagree :)

.collect(Collectors.toList());
Series(String string) {
this.digits =
Arrays.stream(string.split(("")))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would put this on the line above so that it's:

this.digits = Arrays.stream(string.split(""))
            .map(Integer::parseInt)
            .collect(Collectors.toList());

You might know this already but to make sure formatting is correct you can use ctrl + alt + L if you're using intellij on Windows (it's cmd + alt + L on mac). On eclipse it's ctrl + shift + F (cmd + shift + F on mac). I've found that very useful :)

Also string.split(("")) can be string.split(""). Not your fault at all, I just didn't notice it until now :)

Copy link
Copy Markdown
Contributor Author

@ilya-khadykin ilya-khadykin Aug 3, 2017

Choose a reason for hiding this comment

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

@FridaTveit
thanks for your suggestions, I'll update the PR as soon as get back home.

Is there any industry accepted best practices for code formatting (as you probably mentioned) or it's usually specific to the project (if so, please point me to the page describing this rules)?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The defaults in IntelliJ (and probably also Eclipse) should be fine for formatting :) The google style guide formatting section would probably be useful if you want something for reference. I've mostly learnt best practices from experience and code reviews, so I haven't read the google style guide cover to cover, but it looks sensible and I think many people use it :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thanks!

@ilya-khadykin
Copy link
Copy Markdown
Contributor Author

@FridaTveit
updated the PR based on your comments in #discussion_r131078864

Copy link
Copy Markdown
Contributor

@FridaTveit FridaTveit left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you @m-a-ge! :)

@FridaTveit FridaTveit merged commit fe41f1d into exercism:master Aug 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants