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

Team 7 - Fix code smells for the towers #289

Closed
wants to merge 11 commits into from

Conversation

lenhatminh451
Copy link
Contributor

@lenhatminh451 lenhatminh451 commented Oct 16, 2023

This pull request includes changes to reduce the code smells in the tower's tasks, tests and configs. Almost all code smells have been fixed, but still some need to be rechecked on SonarCloud after merging with the main.

@lenhatminh451
Copy link
Contributor Author

I'm going to sleep now, so if anyone sees many approvals from the reviewers, please help me merge this PR to prevent blocking newer PRs. Thank you!

Copy link
Contributor

@ThivanW ThivanW 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 to me!

Copy link
Contributor

@The-AhmadAA The-AhmadAA left a comment

Choose a reason for hiding this comment

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

Would like some clarification on a couple of comments before approving. Also need to resolve merge conflict

private static final short TARGET1 = PhysicsLayer.BOSS; // The type of targets that the Engineer will detect.
private static final short TARGET2 = PhysicsLayer.XENO;
// private static final short TARGET1 = PhysicsLayer.BOSS; // The type of targets that the Engineer will detect.
// private static final short TARGET2 = PhysicsLayer.XENO;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the Bombship not target enemies, and is it handled somewhere else? why has this been commented out?

Copy link
Contributor Author

@lenhatminh451 lenhatminh451 Oct 17, 2023

Choose a reason for hiding this comment

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

This combat task still needs more functionalities; I just commented to check if it can fix the code smells and make it easier for us to continue to develop it later.

// Define a set to keep track of occupied lanes
private static final Set<Integer> occupiedLanes = new HashSet<>();
private static final int COMBAT_TASK_PRIORITY = 2;
private static final int WEAPON_TOWER_MAX_RANGE = 40;
private static final int TNT_TOWER_MAX_RANGE = 6;
private static final int TNT_TOWER_RANGE = 6;
private static final int TNT_KNOCK_BACK_FORCE = 10;
private static final String WALL_IMAGE = "images/towers/wall_tower.png";
private static final String RESOURCE_TOWER = "images/towers/mine_tower.png";
// private static final String WALL_IMAGE = "images/towers/wall_tower.png";
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these paths not necessary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants