Skip to content

Implement Queen Attack generator #393

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
merged 3 commits into from
Sep 5, 2017

Conversation

jpreese
Copy link
Contributor

@jpreese jpreese commented Sep 1, 2017

One thing to note is that our Example for this one isn't quite complete, so I'm going to start that now.

I'm open to suggestions on the generator for this one.. it feels like a lot, but it could just be a more complicated test to generate. Nothing felt too hacky, and most of it made sense to me.

Edit: Example updated to pass new tests.

@jpreese jpreese force-pushed the add-queen-attack-generator branch 2 times, most recently from beb5f6d to d14e62a Compare September 2, 2017 01:13
@ErikSchierboom
Copy link
Member

I'm a bit confused. I would expect the Create method to actually create a queen, not return a boolean value.

@jpreese jpreese force-pushed the add-queen-attack-generator branch from d14e62a to f7eac6f Compare September 2, 2017 12:23
@jpreese
Copy link
Contributor Author

jpreese commented Sep 2, 2017

That was my initial approach as well. I added the Create() method in the stub and had it return type Queen. Though the canonical data has it returning 0 for success, and -1 for failure, I'm not sure why it uses a number for creation, but a boolean for attacking.

Though now that I'm thinking aloud, it may be better to not take the canonical data so literally, and actually go back to Create returning Queen, but throw an exception.

@ErikSchierboom
Copy link
Member

Agreed! The canonical data can be quite weird sometimes.

@jpreese
Copy link
Contributor Author

jpreese commented Sep 2, 2017

Lets give that approach a whirl.

@jpreese
Copy link
Contributor Author

jpreese commented Sep 4, 2017

In case the github ping didn't go off for some reason, this has been changed to throw exceptions.

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.

Looking great! Some small nits.

public void Cannot_occupy_same_space()
public void Queen_with_a_valid_position()
{
var actual = QueenAttack.Create(2, 2);
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps add a comment below this line to indicate that we are testing that no exception is thrown.

return RenderCanAttackAssert(testMethodBody);
}

if(testMethodBody.UseVariableForTested)
Copy link
Member

Choose a reason for hiding this comment

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

Add space after if

{
foreach(var canonicalDataCase in canonicalData.Cases)
{
if(canonicalDataCase.Property == "create")
Copy link
Member

Choose a reason for hiding this comment

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

Add space after if

{
protected override void UpdateCanonicalData(CanonicalData canonicalData)
{
foreach(var canonicalDataCase in canonicalData.Cases)
Copy link
Member

Choose a reason for hiding this comment

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

Add space after foreach

@jpreese
Copy link
Contributor Author

jpreese commented Sep 4, 2017

I really need to remember to update my visual studio formatting preferences.

@ErikSchierboom
Copy link
Member

I really need to remember to update my visual studio formatting preferences.

You really shouldn't have to remember, we should have the tooling do that! I've created an issue for this: #398

@jpreese
Copy link
Contributor Author

jpreese commented Sep 5, 2017

I don't think it's worth making a special template for the initial case of making a comment, so I just added it to the test description.

@ErikSchierboom
Copy link
Member

@jpreese Did you re-run the test generator after making that change? It doesn't appear in the test file :)

@ErikSchierboom ErikSchierboom merged commit fd23081 into exercism:master Sep 5, 2017
@ErikSchierboom
Copy link
Member

Thanks a lot! 🎉

@jpreese jpreese deleted the add-queen-attack-generator branch October 3, 2017 21:45
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