Skip to content

Agustin/langtonant#96

Merged
J0c4 merged 20 commits intodev/langtonantfrom
agustin/langtonant
Feb 10, 2022
Merged

Agustin/langtonant#96
J0c4 merged 20 commits intodev/langtonantfrom
agustin/langtonant

Conversation

@Agustin-Mediotti
Copy link
Copy Markdown
Collaborator

I like it a lot this kata!
I find very good lectures in the process

Copy link
Copy Markdown
Contributor

@J0c4 J0c4 left a comment

Choose a reason for hiding this comment

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

Looks good, please check my comments and let me know your thoughts.

Also, please restore README file that was moved to agustin/ folder (it should stay under langtonant/ folder)

package org.fundacionjala.at15.katas.langtonant.agustin;

public class Ant {
// TODO: Make Ant Class with constructor and methods
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this comments still valid?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It is not, I am verifing for old TODO comments in the code. It should be changed in the next commits.

Grid g;
boolean isFinished;

public Ant(int xPos, int yPos, String direction, Grid g) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Try to avoid variable names with one single letter, they are not descriptive enough to understand what kind of value they store

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ok! refactoring right now. I will take it into consideration in the future.


// direction

if (direction.equals("NORTH")) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The logic of this if-else blocks seem to have a group of instructions that are repeated, you should consider to create a method where the things that change are set using variables/parameters

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes! I was aware of that. Actually, refactoring this part was one of my TODOs. But it seems that I deleted it for a mistake...
However, refactoring right away! 👍🏼

public void run() {
//TODO: while(!isFinished) determine direction, change tile color

while (xPos >= 0 && yPos >= 0 && xPos < g.getWidth() && yPos < g.getHeight() && !isFinished) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this logic take into account a way to specify a number of steps that the ant will take?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It doesnt! Fixing right now.

if (isWhite(h,w)) {
System.out.print("[ ]");
} else {
System.out.print("[x]");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I liked this way of printing the grid 👍🏽

Copy link
Copy Markdown
Contributor

@J0c4 J0c4 left a comment

Choose a reason for hiding this comment

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

Very nice updates!

Please check my comment about changes lost, after that I think this PR can be approved

source = 'src/main/java'
}

task runLangtonAntAdrian (dependsOn: 'classes', type: JavaExec) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The tasks for Adrian and Adhemar are being deleted, maybe this happened during conflicts resolution. Please restore those tasks (it can be a new commit) so the code from them is not lost

Same for the java files under their folders that were deleted

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I didn't realize that! Their files were restored as well their tasks.

+ ", " + "Color: " + (grid.isWhite(xPos, yPos) ? "BLACK" : "WHITE"));
}

public void moveNorth() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Much more legible! 😃

Copy link
Copy Markdown
Contributor

@J0c4 J0c4 left a comment

Choose a reason for hiding this comment

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

One last comment


task runLangtonAntAdhemar (dependsOn: 'classes', type: JavaExec) {
main = 'org.fundacionjala.at15.katas.langtonant.adhemar.Main'
classpath = sourceSets.main.runtimeClasspath
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please also restore this line, it might break Adhemar's task

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@J0c4 J0c4 merged commit 135b4dc into dev/langtonant Feb 10, 2022
@J0c4 J0c4 deleted the agustin/langtonant branch February 14, 2022 12:02
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