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

Add mentoring notes for java/reverse-string #1006

Merged
merged 1 commit into from
Jul 5, 2019

Conversation

michael-berger-FR
Copy link
Contributor

Already discussed in PR #991

@SleeplessByte SleeplessByte requested a review from a team May 29, 2019 10:06
Copy link
Contributor

@sshine sshine left a comment

Choose a reason for hiding this comment

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

Thanks. I've approved it as it is, but left a few suggestions for whitespace formatting.

I'm sorry for letting this wait; I wasn't automatically notified because of the new PR.

String reverse(String inputString) {
String output = "";
for (int i = inputString.length() - 1; i >= 0; i--) {
output += inputString.charAt(i);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
output += inputString.charAt(i);
output += inputString.charAt(i);

Indentation by 4 spaces.

class ReverseString {
String reverse(String inputString) {
final int length = inputString.length();
return IntStream.iterate(length-1, i-> i-1).limit(length)
Copy link
Contributor

@sshine sshine Jun 6, 2019

Choose a reason for hiding this comment

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

Suggested change
return IntStream.iterate(length-1, i-> i-1).limit(length)
return IntStream.iterate(length - 1, i -> i - 1)
.limit(length)

Whitespace in lambda, align all chained methods.

final int length = inputString.length();
return IntStream.iterate(length-1, i-> i-1).limit(length)
.mapToObj(inputString::charAt)
.collect(StringBuilder::new, StringBuilder::append, StringBuilder::append).toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.collect(StringBuilder::new, StringBuilder::append, StringBuilder::append).toString();
.collect(StringBuilder::new, StringBuilder::append, StringBuilder::append)
.toString();

Align all chained methods.

@jmrunkle
Copy link
Contributor

Not exactly sure how to do this, but I wanted to mention that this PR Fixes #1101

@sshine
Copy link
Contributor

sshine commented Jul 5, 2019

@michael-berger-FR: Hello. :-)

It seems that I cannot push to your feature branch.

Would you like to address my suggested changes?

@sshine
Copy link
Contributor

sshine commented Jul 5, 2019

@jmrunkle: I think it's okay to not have a corresponding issue for each PR if the discussion is already documented here.

@sshine sshine merged commit 88f0f67 into exercism:master Jul 5, 2019
@sshine
Copy link
Contributor

sshine commented Jul 5, 2019

I've committed the mentor notes sans whitespace formatting on the premise that they can be fixed later.

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.

None yet

5 participants