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

Improve formatting of multidimensional arrays (#991) #1044

Conversation

Projects
None yet
3 participants
@DmitriyMaksimov
Copy link
Contributor

commented May 13, 2019

  • Implemented formatting of multidimensional arrays (MultidimensionalArrayFormatter)
  • Added unit test for the formatter
  • Supported formatting of multidimensional arrays with bounds
  • Supported formatting of n-ary dimensional arrays
@DmitriyMaksimov

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Test When_no_exception_should_be_thrown_after_wait_time_but_it_was_it_should_throw failed - think the test is "blinking" it has failed on my PC few times as well. Don't see why my changes could affect the test.

@dennisdoomen
Copy link
Member

left a comment

Thanks for the work you've done. IMHO, it needs a bit of cleaning. I hope you're willing to spend a bit more time on this.


var indecies = Enumerable.Range(0, arr.Rank).Select(dimention => arr.GetLowerBound(dimention)).ToArray();

bool IsFirstIteration(int index, int dimention)

This comment has been minimized.

Copy link
@dennisdoomen

dennisdoomen May 13, 2019

Member

Please don't use local functions. This method is already extremely long and quite difficult to follow.

{
var arr = (Array)value;

if (arr.Length <= 0)

This comment has been minimized.

Copy link
@dennisdoomen

dennisdoomen May 13, 2019

Member

Can this ever be negative?

}

/// <inheritdoc />
public string Format(object value, FormattingContext context, FormatChild formatChild)

This comment has been minimized.

Copy link
@dennisdoomen

dennisdoomen May 13, 2019

Member

Any chance you can break it down into smaller methods so that the algorithm is more clearer?

result.Should().Match("{{{'1-5-7', '1-5-8', '1-5-9', '1-5-10'}, {'1-6-7', '1-6-8', '1-6-9', '1-6-10'}, {'1-7-7', '1-7-8', '1-7-9', '1-7-10'}}, {{'2-5-7', '2-5-8', '2-5-9', '2-5-10'}, {'2-6-7', '2-6-8', '2-6-9', '2-6-10'}, {'2-7-7', '2-7-8', '2-7-9', '2-7-10'}}}".Replace("'", "\""));
}

public static IEnumerable<object[]> MultiDimentionalArrayData =>

This comment has been minimized.

Copy link
@dennisdoomen

dennisdoomen May 13, 2019

Member

Please put this directly below the test method that needs it.

@@ -748,6 +748,65 @@ public void When_formatting_a_completion_source_it_should_include_the_underlying
result.Should().Match("*TaskCompletionSource*Task*System.Int32*Status=WaitingForActivation*");
}

[Theory]
[MemberData(nameof(MultiDimentionalArrayData))]
public void When_formatting_a_multi_dimentional_array_it_should_show_structure(object value, string expected)

This comment has been minimized.

Copy link
@dennisdoomen

dennisdoomen May 13, 2019

Member

Can you please move these into a dedicated specs class? Most of what's here is pretty old and still needs to be split.

Changes requested on code review:
* Unit tests moved into a dedicated specs class
* MemberData moved directly below the test method that needs it
* Use private methods instead of local functions
@DmitriyMaksimov

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

@dennisdoomen Thanks for your code review!
I made requested changes except "Any chance you can break it down into smaller methods so that the algorithm is more clearer?" - it's a tricky one. Actually it's a while loop which emulates n-ary loop - have no idea how to split it. Saying that, I moved local functions out, so now the method is 58 lines and half of them are empty lines/brackets, so not as bad as used to be.
Please let me know if you spot another issue(s).

@jnyrup
Copy link
Collaborator

left a comment

The implemented algorithm and tests looks good to me.
I especially liked that you cover the case, where the array does not start at index 0.

I've given some comments which I think could help making the intent of the algorithm more clear.


var sb = new StringBuilder();

var indecies = Enumerable.Range(0, arr.Rank).Select(dimention => arr.GetLowerBound(dimention)).ToArray();

This comment has been minimized.

Copy link
@jnyrup

jnyrup May 15, 2019

Collaborator

How about dimensionIndices?


var sb = new StringBuilder();

var indecies = Enumerable.Range(0, arr.Rank).Select(dimention => arr.GetLowerBound(dimention)).ToArray();

This comment has been minimized.

Copy link
@jnyrup

jnyrup May 15, 2019

Collaborator

dimention ->dimension

return sb.ToString();
}

private bool IsFirstIteration(Array arr, int index, int dimention)

This comment has been minimized.

Copy link
@jnyrup

jnyrup May 15, 2019

Collaborator

dimention ->dimension

return index == arr.Rank - 1;
}

private bool IsLastIteration(Array arr, int index, int dimention)

This comment has been minimized.

Copy link
@jnyrup

jnyrup May 15, 2019

Collaborator

dimention ->dimension

return index == arr.GetLowerBound(dimention);
}

private bool IsMostInnerLoop(Array arr, int index)

This comment has been minimized.

Copy link
@jnyrup

jnyrup May 15, 2019

Collaborator

IsMostInnerLoop -> IsInnerMostLoop

return index >= arr.GetUpperBound(dimention);
}

private bool IsNotLastIteration(Array arr, int index, int dimention)

This comment has been minimized.

Copy link
@jnyrup

jnyrup May 15, 2019

Collaborator

🙃 I would leave out that function and just use !IsLastIteration directly.

// Emulate n-ary loop
while (currentLoopIndex >= 0)
{
var loopValue = indecies[currentLoopIndex];

This comment has been minimized.

Copy link
@jnyrup

jnyrup May 15, 2019

Collaborator

Would currentDimensionIndex be a more describing name for loopValue?

sb.Append(formatChild(string.Join("-", indecies), enumerator.Current));
if (IsNotLastIteration(arr, loopValue, currentLoopIndex))
sb.Append(", ");
++indecies[currentLoopIndex]; // Increment loop variable

This comment has been minimized.

Copy link
@jnyrup

jnyrup May 15, 2019

Collaborator

Please put comments right above the statement.

if (currentLoopIndex < 0)
break;

// update loopValue and loopMaxValue

This comment has been minimized.

Copy link
@jnyrup

jnyrup May 15, 2019

Collaborator

loopMaxValue does not exist.
From CSharpGuidelines

Try to focus comments on the why and what of a code block and not the how. Avoid explaining the statements in words, but instead help the reader understand why you chose a certain solution or algorithm and what you are trying to achieve. If applicable, also mention that you chose an alternative solution because you ran into a problem with the obvious solution. ...


var sb = new StringBuilder();

var indecies = Enumerable.Range(0, arr.Rank).Select(dimention => arr.GetLowerBound(dimention)).ToArray();

This comment has been minimized.

Changes requested on code review:
* Renamed variables
* Removed redundant comments
* Use explicit type instead of `var`
@DmitriyMaksimov

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

@jnyrup Thanks for your comments!
I made requested changes - they are improved code readability significantly.
Please review when you have a chance.

@jnyrup

jnyrup approved these changes May 15, 2019

@jnyrup jnyrup merged commit 0ed6bbd into fluentassertions:master May 15, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@jnyrup

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2019

@DmitriyMaksimov Thanks for the contribution - Fluent Assertions just got one tad better.
Look out for your name in the next release.

@DmitriyMaksimov

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

Happy to contribute 😄
It's my first, but hope not last contribution into FluentAssertions.
Thank you very much for guiding me along the process!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.