Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Feb 14, 2020

Summary of the Pull Request

The origin is 0, 0. The X and Y, by definition, must be set to 0, which is the same in both .X and .Y members. Because of this, it makes more sense to chain assign them to 0.

Changes were tested manually and automatically.

References

PR Checklist

  • Closes #xxx
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Manual Testing
Automated Testing

@beviu
Copy link
Contributor

beviu commented Feb 15, 2020

This kind of a joke (not serious), but here are very scientific counter arguments 😆 :

  • What about the fact that because chain assignments are rare, when you stumble upon one, you are surprised for a few ms and you have to stop and think about what it does?
  • When reading the original code you don't have to move your eyes more than once because you can just look at the center of the block and see the code around it thanks to the arrangement cones in the center of the eye. But now you can't: while the total surface of the block is smaller, the code block is large in one axis so you have to move your eyes at least twice horizontally to scan the whole block.

BTW, I wonder why this:

_dwScreenBufferSize.X = 80;
_dwScreenBufferSize.Y = 25;
_dwWindowSize.X = _dwScreenBufferSize.X;
_dwWindowSize.Y = _dwScreenBufferSize.Y;
_dwWindowSizePixels = { 0 };
_dwWindowOrigin.X = _dwWindowOrigin.Y = 0;
_dwFontSize.X = 0;
_dwFontSize.Y = 16;
ZeroMemory((void*)&_FaceName, sizeof(_FaceName));
wcscpy_s(_FaceName, DEFAULT_TT_FONT_FACENAME);
_CursorColor = Cursor::s_InvertCursorColor;
_CursorType = CursorType::Legacy;

isn't done inside of the initialization list or inside the header file (src/host/settings.hpp). Maybe it would be even better to put it in the initialization list or the header file?
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-in-class-initializer

…d make its context clearer.

The origin is 0, 0. The X and Y, by definition, must be set to 0, which is the same in both .X and .Y members. Because of this, it makes more sense to chain assign them to 0.
@ghost ghost requested a review from DHowett-MSFT February 16, 2020 18:49
@DHowett-MSFT
Copy link
Contributor

Hey @pi1024e! Thanks for the contribution!
I appreciate what you've done here, but I'd like to see something more broadly set up across the codebase instead of one-off changes here and there. We'd like to converge on a final style, but it's easiest to do that once we build consensus for how that style should look.

We usually avoid double assignment because it does make for harder-to-read code.

For now, until we have more discussion, I'm going to close this one out.

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