justgo13 / sysc3110-risk Goto Github PK

• Please make sure everyone commits and pushes their own code to GitHub (and that you don't rely on other members to commit code for you). This helps me make sure that everyone is contributing evenly and that everything is fair! You will be deducted marks in the next Milestone if the repository shows you did not commit much (regardless of if someone else submitted code on your behalf!).

• This is also important because I need to see that everyone knows how to use GitHub properly (which is a learning objective of the project)

From cuLearn:

You have a lot of logic in your controller! (This is a code smell)

Controllers should be "thin", i.e. very few lines of code. The logic in the controller should either delegate to the model or the view. When there is a lot of code in the controller, it is a sign that your mode/view/controller are tightly coupled and that the controller is doing work that belongs to the model or view.
To fix this: imagine you have two different types of views. You don't actually have to do this, but how would your design/code change for this? Make use of model listeners in the model and the controller should always delegate logic to the model (and sometimes the view)

Let me know if you have questions about this!

1. Make sure you don't have methods that are handling way too much logic, such as RiskGame.processCommand(..). Try breaking methods like these down into sub-methods that handle different parts of the logic.

2. Document your methods and code a bit more. Especially the methods such as processCommand(..) which are responsible for large pieces of game logic!

You've definitely taken a step in the right direction! You still have a few things that should be addressed, and my feedback for your RiskController class is as follows:

1. Nitpick: Enums are classes, so they should follow the correct naming conventions (should be "AttackState" instead of "attackState")
   

   SYSC3110-Risk/src/RiskController.java

   Line 17 in 68a0a5c

   private enum attackState {

2. You have state in your controller: the whole point of a model is to maintain the state, so the following variable and any corresponding logic should be in the model. If you need to get the current state from the model, then you should be doing something like: model.getAttackState()
   

   SYSC3110-Risk/src/RiskController.java

   Line 20 in 68a0a5c

   private attackState currentState;

3. I'd consider the following to be "smelly". Even though you are delegating to the model for the logic (good!), the following code is still very mangled (with lots of if/else if/else statements) which makes it hard to understand your code. Some of this logic could easily be broken down into sub-methods and be put into the model. It should also be the responsibility of the model to change the state (you shouldn't set attackState in the controller).
   

   SYSC3110-Risk/src/RiskController.java

   Lines 30 to 64 in 68a0a5c

   	if(e.getActionCommand().equals("country")) {
   	CountryButton countryButton = (CountryButton) e.getSource();
   	// Dealing with countries being clicked
   	//identify what turn it is
   	if (currentState == attackState.SHOW_DEFENDING_COUNTRIES) {
   	attackingCountry = countryButton.getCountry();
   	troopCount = riskView.showDefendingCountries(e);
   	currentState = attackState.COMMENCE_ATTACK;
   	} else if (currentState == attackState.COMMENCE_ATTACK) {
   	defendingCountry = countryButton.getCountry();
   	riskModel.attack(attackingCountry, defendingCountry, troopCount);
   	riskView.performAttack(attackingCountry);
   	currentState = attackState.SHOW_DEFENDING_COUNTRIES;
   	}
   	}else if (e.getActionCommand().equals("endturn")){
   	if (currentState == attackState.SHOW_DEFENDING_COUNTRIES || currentState == null) {
   	// disable all attacking countries of current player
   	ArrayList<CountryButton> countryButtons = riskView.convertCountryToCountryButtons(riskModel.getAttackingPlayer().getCountriesOwned());
   	countryButtons.forEach(countryButton -> countryButton.setEnabled(false));
   	// moves to next player
   	riskModel.endTurnPhase(riskModel.getBoard().getPlayers().get(riskModel.getTurnIndex()).getId());
   	attack.setEnabled(true);
   	}
   	} else if (e.getActionCommand().equals("attack")){
   	// show all countries that we can attack with
   	// this button should get disabled after until the attacking phase has been completed
   	currentState = attackState.SHOW_PLAYER_COUNTRIES;
   	attack = (JButton) e.getSource();
   	attack.setEnabled(false);
   	if (currentState == attackState.SHOW_PLAYER_COUNTRIES) {
   	riskView.showAttackingCountries();
   	currentState = attackState.SHOW_DEFENDING_COUNTRIES;
   	}
   	}

4. You still have "magic" strings hard-coded into your program. You should be using an enum or constant for representing Strings like "country". This is a code smell and makes your code unmaintainable!

   • You have lots of magic numbers and strings in your code. The next milestone I will be very strict with code smells so make sure you don't have any magic numbers/strings like the following ("country")
     

     SYSC3110-Risk/src/RiskController.java

     Line 30 in 68a0a5c

     if(e.getActionCommand().equals("country")) {

Let me know if you have any questions or if you want more feedback :) Hope this helps!