kyrylo-shyvam / dass-ass-2 Goto Github PK

• Townhall class is imported twice in game.py
• It is also used as a singleton(?) in
  

  dass-ass-2/src/game.py

  Line 146 in 407bae3

  Townhall.health = (3/2)*Townhall.health

Suggested refractoring

• Find use of both the classes
• Name them properly and/or remove unused stuff
• Fix import

Barbarians choose the same wall as the new target after they destroy a Townhall wall. The result of this is that all the Barbarians don't move and are stuck when their target wall is destroyed.

The reason for this is that we don't set self.death property of wall to be True when a wall is destroyed unlike other buildings

Fix:
Set the self.death property to be True :

        if(self.health <= 0):
            self.color = Back.LIGHTGREEN_EX + " " + Style.RESET_ALL
            self.death = True

Buildings like cannon, townhall etc. inherit from the Building class, but this abstraction is not used properly. The aim was to have a static list of all buildings and all its base types for easy querying.

Code smell: Unexploited encapsulation, duplicated code

Suggested refractor

• Base abstract entity class, which has init, render(colorArray, idArray) and tick() function
• Game class should support querying active entities by their type

Colorama should be initialized only once, and by setting autoreset to True.

In many places all over the code, lines like this exist

The code assumes height and width, instead it should be using values rows and columns.

This issue is the single place for all instances like this.

Code smell: Excessive use of literals

There is a lot of duplicated code with just the color changed for each health level in the Townhall class render function

Suggested Refactor:

• Make a new function which takes the color as input and marks the colorarray for Townhall to that color

Code smell: Duplicate code

Suggested refractor

• One function which takes a list as input, or,
• Base class for objects that can be rendered, then we keep just one list for all active objects and render them

• do a fork
• add this repo as upstream using git remote add upstream [email protected]:Kyrylo-Shyvam/dass-ass-2.git
• do git pull --rebase upstream master
• work in master/feature branch
• do git push origin branch
• squash(in case many commits) merge PR yourself if small or wait for review
• after merging do git pull --rebase upstream master because the history will be different

Do only meaningful commits, dont make a commit after every change. If you do then squash them in one.

The following lines modify the terminal to enable raw mode. But the game doesn't reset the settings.

Fix: keep the old_settings and set them when exiting.

The game class only prints the colors filled with other classes. It does nothing else. All the other functionality is in game.py.

Code Smell: Lazy class/freeloader and Mysterious name

Suggested Refractor

• Rename this class to something like Renderer or,
• Move actual game logic to this class, making the game.py just create an instance of Game and call its play() or run() function

• King sometimes is rendered as two blocks at different positions instead of one long rectangle.
• Size of the map changes after some time.
• Weird rendering of right boundaries.
• Moving king also moves other objects

Fix: I think the screen isn't cleared properly before rendering. We should first investigate that, and then investigate individual classes. We can also make a class which contains methods for filling a specified area on the console with a specified color. So the rendering code is at one place only.

Related: #7 #8 #12

When king touches any boundary, then it wont be able to move in the direction opposite to the boundary. Lets say the king is touching the left boundary, then it wont be able to move right. Similar for boundaries in all direction. Touching any corner will disable movement all together.

Fix: Read and fix kings movement code.

A method called spawn is imported on line 2:

Also, this function has a lot of duplicate code which can easily be fixed.

Code Smell: ambiguous definition and Repetitive code.
Suggestion: Delete the unused import and remove repetitions.

healStart function is invoked when h key is pressed:

The following code is run inside healStart which crashes the game:

The reason for the crash is that Townhall has no attribute health which we are trying to access

Fix:
Change Townhall to townhall in the above code because Townhall is the class while townhall is an instance of that class with health property

Inside move method in barbarians.py.

Code Smell: Conditional complexity and repetitive code.

Suggested Refactoring:

1. Remove the large code block inside the if statement and keep and if statement at the top with a return inside.

if(self.target == None):
   return idArray

2. Use a for loop to iterate through all the possible movements instead of repetitive conditional code.

Code smell: Many branches(cyclomatic complexity), long lines(god lines)

Suggested refractor

• Find and refractor common code into functions
• Format properly

Hold c to spawn barbarians. Due to the print statements the game is no longer or partially visible.

Fix: Remove the print statements

This function has no relation to king class. It can just be a utility function.

Suggested Refractor

• Move it out of king class into some utility class.