-
Notifications
You must be signed in to change notification settings - Fork 4
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
Conversation
… in TowerCombatTask
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! |
There was a problem hiding this 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!
There was a problem hiding this 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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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?
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.