Skip to content

Added Minesweeper generator #472

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

Merged

Conversation

felix91gr
Copy link
Contributor

@felix91gr felix91gr commented Oct 11, 2017

  • Added generator itself
  • Updated generated Test file
  • Fixed Example and Minesweeper classes to recieve and return the right type on method Annotate.
    Was: string -> string. Now it is: string[] -> string[]

@ErikSchierboom please tell me I didn't screw up by changing Annotate from a string -> string function to a string[] -> string[] one? I don't know how else to interpret the Canonical Data files.

If that's allright, then this commit is pretty much cooked afaik.

@felix91gr
Copy link
Contributor Author

I branched this one off of master, so it's clean to be merged before #471 and #412

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

I think it's perfectly fine to change the input and output to use a string[]. There are two things I thing can be improved:

  1. Use variables for the input and expected output. They are currently displayed inline, which leads to long sentences that are hard to read.
  2. Format the arrays such that they are neatly aligned on different lines. For an example, see this PR.

Thanks for doing this!

@felix91gr felix91gr force-pushed the adding-minesweeper-generator branch from a460451 to e03fa85 Compare October 11, 2017 19:31
@felix91gr
Copy link
Contributor Author

I'm done with number 1.

Format the arrays such that they are neatly aligned on different lines.

Now I'm on to this.

@felix91gr felix91gr force-pushed the adding-minesweeper-generator branch 2 times, most recently from b8c088e to 0f4acdd Compare October 11, 2017 19:40
@felix91gr
Copy link
Contributor Author

@ErikSchierboom this one's ready as well ;)

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

One tiny comment, and then we're good to go!

{
var input = "";
var expected = "";
var input = new []
Copy link
Member

Choose a reason for hiding this comment

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

I think no rows should mean an empty array, not an array with an empty string. Could you replace that (also for the expected value)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

- Added generator itself
- Updated generated Test file
- Fixed Example and Minesweeper classes to recieve and return the right type on method Annotate.
  Was: string -> string. Now it is: string[] -> string[]

Changed presentation on Minesweeper test suite
- Inputs and Expected output are created as variables for better readability
- Inputs and Expected output are formatted as multiline string arrays, for better readability.
- Polish: now test suite creates strict string arrays.
@felix91gr felix91gr force-pushed the adding-minesweeper-generator branch from 0f4acdd to 81877a7 Compare October 11, 2017 20:20
@felix91gr
Copy link
Contributor Author

Okay, now I've fixed and tested it. The arrays it creates now are actually better, because they are strictly new string[].

Creating just new [] is not a good idea imo, if we can avoid it. In this case, we know that the input is exactly a string array, so I changed that as well.

Thanks for the feedback.
Tell me what you think of this one :)

@ErikSchierboom ErikSchierboom merged commit a634d4c into exercism:master Oct 12, 2017
@ErikSchierboom
Copy link
Member

Merged. Thanks! 🎉

@felix91gr felix91gr deleted the adding-minesweeper-generator branch October 12, 2017 18:51
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