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

Change tile width dynamically to allow longer strings #56

Closed
wants to merge 8 commits into from
Closed

Change tile width dynamically to allow longer strings #56

wants to merge 8 commits into from

Conversation

Scronkfinkle
Copy link

The table will now draw to the longest length number with a small border of spaces

Resolves #54

@cawvyoct
Copy link
Collaborator

Thank you for your contribution! 🎉
The following will be a quick code review, ok?

Copy link
Collaborator

@cawvyoct cawvyoct left a comment

Choose a reason for hiding this comment

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

Once the changes have been made, I think the code can be accepted.
This will help in preparing for PR#52 .

src/game.cpp Outdated Show resolved Hide resolved
src/game.cpp Outdated Show resolved Hide resolved
src/headers/game.hpp Outdated Show resolved Hide resolved
@Scronkfinkle
Copy link
Author

Alright, I'll fix it up when I get home from work in a few hours

Scronkfinkle and others added 8 commits October 24, 2018 08:58
The table will now draw to the longest length number with a small border of spaces

Resolves #54
Default width of cell shall remain 4+2=6 characters width. Cell
width and border string are calculated only when largestTile changes
value when largestTileLength is greater than 4.
src/game.cpp Outdated
clearScreen();
drawAscii();
drawScoreBoard(std::cout);

int numlen = std::to_string(largestTile).length();
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

src/game.cpp Outdated
std::string border;
// Multiple cell border enum by 2 so it applies the same padding
// to both sides
for(int i = 0; i < numlen+( CELL_BORDER * 2); i++){
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 & for comment too.

@cawvyoct
Copy link
Collaborator

So, things are a bit confusing here with the authors of commits in this PR.
Suggestions of "things to fix" before hard edits would have been preferred...
(This fosters encouragement of committer to correct missing tasks using own abilities).

@plibither8
Copy link
Owner

My apologies @cawvyoct 😐! I saw a few things that could be counted as quick-fixes so I did it myself. I'll keep this in mind, thank you.

@@ -55,6 +55,8 @@ class Game {
ull score;
ull bestScore;
ull largestTile;
int cellWidth;
std::string horizontalBorder;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better practice would be:

  • A free anonymous-namespace function that outputs a horizontalBorder string.

int largestTileLength = std::to_string(largestTile).length();
if (cellWidth > MINIMUM_CELL_WIDTH) {
cellWidth = largestTileLength;
horizontalBorder.append("─");
Copy link
Collaborator

Choose a reason for hiding this comment

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

With an anonymous-namespace free function, you can take in a parameter [length] and the string returned would be a string of the [horizontalBorder character].

@@ -228,7 +220,7 @@ void Game::drawBoard() {
std::cout << " └";

for (int i = 0; i < gameBoardPlaySize; i++) {
std::cout << border;
std::cout << horizontalBorder;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Everytime you want to draw the [horizontalBorder string], you would just call the function.

bestScore(0), moveCount(-2), largestTile(2), stateSaved(false),
noSave(false) {}
bestScore(0), moveCount(-2), largestTile(2), cellWidth(4),
horizontalBorder("──────"), stateSaved(false), noSave(false) {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This means one would not need the [horizontalBorder] member-variable.

@Scronkfinkle
Copy link
Author

@plibither8 Do you want to handle these changes or shall I? I don't mind either way, I just don't want us both to accidentally push upstream at once

@plibither8
Copy link
Owner

Go ahead, I guess it's better since you've created the PR. I have some other work right now, won't be able to work on this until later. Thanks 👍

@cawvyoct
Copy link
Collaborator

constexpr int CELL_BORDER = 1;

This works too, however enums are great for newer developers to understand that some variables have a group context together.

@cawvyoct
Copy link
Collaborator

cawvyoct commented Nov 2, 2018

Hi @Scronkfinkle,
Just an update, but thanks for your contributions so far to the project! 👍
Currently, the game's codebase is being refactored quite a bit and you will need to update your PR to match the new refactored code (when code refactoring slows down quite a bit).

Dynamic-width tiles are still very much a challenge open for you if you wish. I think even when more refactored code lands into the codebase, your skill / help would be fine in completing it. 🐱

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.

Change tile width dynamically to allow longer strings
3 participants