Adding approach to Queen-Attack #2860
Conversation
|
@jagdish-15 Have you looked at https://exercism.org/docs/building/configlet/uuid |
|
@tasxatzial, thank you so much for letting me know! I couldn't find it anywhere—I guess I didn't search thoroughly enough in the documentation. But I've now configured the configlet and will be adding the UUID as soon as possible! |
jagdish-15
force-pushed
the
add-approach-queen-attack
branch
from
e0f7eb5
to
c8acf18
Compare
|
Just by looking at the general structure of the files, it's probably a good idea to format them with one sentence per line. The maintainer will likely ask for it, see here |
|
@tasxatzial, I've formatted the document as you requested. Thank you for the suggestions! Please let me know if there's anything else I should consider. |
|
@jagdish-15 Well, I can do a quick review of the general style (not the code), but I'd prefer to get permission from the maintainer first. Keep in mind that any changes I propose can be overridden by the maintainer, so I'll need to be extra careful with my suggestions to minimize your workload. @kahgoh let me know if you need any help with this one. |
|
Thanks, @tasxatzial! I appreciate your offer to review the style. I understand that any changes might be overridden by the maintainer, so I’ll leave it to you to proceed carefully. @kahgoh, please let me know if you have any other feedback or if you'd like me to make any changes. |
exercises/practice/queen-attack/.approaches/simple-comparison/content.md
Outdated
Show resolved
Hide resolved
exercises/practice/queen-attack/.approaches/simple-comparison/content.md
Outdated
Show resolved
Hide resolved
exercises/practice/queen-attack/.approaches/simple-comparison/content.md
Outdated
Show resolved
Hide resolved
|
Thanks @jagdish-15, I've just gotten around to taking a look and posting the comments. Also thanks @tasxatzial for reviewing the style! |
kahgoh
changed the title
Co-authored-by: Kah Goh <[email protected]>
Co-authored-by: Kah Goh <[email protected]>
…content.md Co-authored-by: Kah Goh <[email protected]>
…ing the content for the same
|
Hello @kahgoh, I've renamed the approach to the one you suggested, as it better aligns with the content and the overall approach (thank you for that!). Additionally, I've streamlined the content file by removing several unnecessary sections and making alterations to others. Could you take a look at the changes and let me know what you think? |
|
|
||
| ## Explanation | ||
|
|
||
| 1. **Constructor**: |
There was a problem hiding this comment.
Is meant to be a sub-heading (###)? If so could you also fix move remove the indenting of the text in the section?
| 1. **Constructor**: | |
| ### Constructor |
|
|
||
| If either of these conditions is true, an exception is thrown. | ||
|
|
||
| 2. **Method (`canQueensAttackOneAnother`)**: |
There was a problem hiding this comment.
Similar to above, I think this is a heading? Also suggest rearranging the name to try make it read better.
| 2. **Method (`canQueensAttackOneAnother`)**: | |
| ### `canQueensAttackOneAnother` Method |
There was a problem hiding this comment.
I had a review pending, but most of the suggestions have been covered. One last thing i'd like to point out is that there are lots of ifs that are redundant since it's clear that the sentence describes a condition. I'll leave them as suggestions.
@jagdish-15 don't act on them without permission from the maintainer.
| - If either queen is `null`. | ||
| - If both queens occupy the same position. |
There was a problem hiding this comment.
| - If either queen is `null`. | |
| - If both queens occupy the same position. | |
| - Either queen is `null`. | |
| - Both queens occupy the same position. |
| - If the row difference is zero (the queens are on the same row). | ||
| - If the column difference is zero (the queens are on the same column). | ||
| - If the row and column differences are equal (the queens are on the same diagonal). |
There was a problem hiding this comment.
| - If the row difference is zero (the queens are on the same row). | |
| - If the column difference is zero (the queens are on the same column). | |
| - If the row and column differences are equal (the queens are on the same diagonal). | |
| - The row difference is zero (the queens are on the same row). | |
| - The column difference is zero (the queens are on the same column). | |
| - The row and column differences are equal (the queens are on the same diagonal). |
| 1. **Same Row**: If the queens are on the same row. | ||
| 2. **Same Column**: If the queens are on the same column. | ||
| 3. **Same Diagonal**: If the queens are on the same diagonal, i.e., the absolute difference between their row and column positions is equal. |
There was a problem hiding this comment.
| 1. **Same Row**: If the queens are on the same row. | |
| 2. **Same Column**: If the queens are on the same column. | |
| 3. **Same Diagonal**: If the queens are on the same diagonal, i.e., the absolute difference between their row and column positions is equal. | |
| 1. **Same Row**: The queens are on the same row. | |
| 2. **Same Column**: The queens are on the same column. | |
| 3. **Same Diagonal**: The queens are on the same diagonal, i.e., the absolute difference between their row and column positions is equal. |
The suggestions look fine to me. |
…h-15/java into add-approach-queen-attack Merging with remote
|
Thank you so much, @tasxatzial and @kahgoh, for your valuable feedback! I truly appreciate it and have updated the content based on all your suggestions. Apologies for the delay in getting back to you! |
pull request
This pull request adds an approach to the Queen-Attack exercise.
I was unsure how to generate a
uuidfor the approach, so I've left it empty for now. I would greatly appreciate guidance on how to generate one.Reviewer Resources:
Track Policies