Global code refactor

[torbacz]

Description

• Code refactor.

Motivation and Context

• Addresses Improve performances for TestFramework and Json  Home#1130.

How Has This Been Tested?

• Local tests from POC project.

Screenshots

Types of changes

• Improvement (non-breaking change that improves a feature, code or algorithm)
• Bug fix (non-breaking change which fixes an issue with code or algorithm)
• New feature (non-breaking change which adds functionality to code)
• Breaking change (fix or feature that would cause existing functionality to change)
• Config and build (change in the configuration and build system, has no impact on code or features)
• Dependencies (update dependencies and changes associated, has no impact on code or features)
• Unit Tests (add new Unit Test(s) or improved existing one(s), has no impact on code or features)
• Documentation (changes or updates in the documentation, has no impact on code or features)

Checklist:

• My code follows the code style of this project (only if there are changes in source code).
• My changes require an update to the documentation (there are changes that require the docs website to be updated).
• I have updated the documentation accordingly (the changes require an update on the docs in this repo).
• I have read the CONTRIBUTING document.
• I have tested everything locally and all new and existing tests passed (only if there are changes in source code).
• I have added new tests to cover my changes.

[Ellerbach]

Thanks for the refactoring. I bet you are intensively testing all the changes, correct? :-)

[torbacz]

@Ellerbach
This one, not yet. Can't run tests project locally. That's why it's draft :)

[josesimoes]

@torbacz why is that? Any error message or behavior that you're seeing to help understand this?

[torbacz]

First standard issues with local tests - fixed by adjusting nfproj and nano.runsettings.

Now

[torbacz]

@josesimoes
I know it may be a confusing at first but it's common refactoring technique to keep code clean. For simple conditions like here it may not make sens right now, but if you add few more conditions in future it easier to read that way. Check this article: https://betterprogramming.pub/refactoring-guard-clauses-2ceeaa1a9da (you can skip text, just check code snippets)
Also, when there is less code the reader is happier 😄

But If you want, I can revert this changes. Up to you.

[torbacz]

And the "POC" tests results

[Ellerbach]

Can you add a test in the POC with the data row? Not sure there is one. And show how it is displayed then in the screen capture. Just want to make sure that one will work as well.

On the refactoring and reverse conditions, it's a matter of habits. I'm not the biggest fan neither but if this is a bit more simple and still work, I'm ok with it.

[torbacz]

@Ellerbach
Already there, check the results from previous screen. They are displayed as "MethodName(index[0-x])"
Again, can't run tests locally... "No suitable test runner found"

[josesimoes]

@josesimoes I know it may be a confusing at first but it's common refactoring technique to keep code clean. For simple conditions like here it may not make sens right now, but if you add few more conditions in future it easier to read that way. Check this article: https://betterprogramming.pub/refactoring-guard-clauses-2ceeaa1a9da (you can skip text, just check code snippets) Also, when there is less code the reader is happier 😄

Thanks for sharing. I understand some of Carlos points there, but I don't necessarily agree with all of them. 😁

In particular in this PR: I'm OK with your changes applying to those large code refactors on which you switch the IF clause and return immediately, then the large code block comes next. A totally different thing is applying that to those small code blocks. On those I really think that keeping the "positive" IF and ELSE do improve code readability. I admit this is very much related with code style and personal preference, so let's ask for another opinion and move forward with this. 😉
@Ellerbach care to weigh in?

[Ellerbach]

@josesimoes you have a code style preference, I have a code style preference (close to yours), @torbacz have a code style preference. Everyone has. And we don't have specific rules for this one about checking conditions and imbricated if. We do have others commonly admitted with explicit bracelet, naming conventions, documentation, etc.
I am fine with this rework of the code because I still can read, yes, I have a make a little bit more efforts because of the reverse checks but that's ok. And there are optimizations we wanted (like typeof checks for the types, etc).
And the code is clean, readable.
So fine with this!

[Ellerbach]

Thanks for the adjustments even if code style is a bit different than mine :-)

[josesimoes]

LGTM