Game logic in mvc: model or controller?

Question

Sorry if the title is too broad, I'll try to explain this in a more specific way down here.

We need to create a board-game in Java for an University project and we have to use the mvc pattern for it.

We found helpful to view this board game as a Finite State Machine, so we implemented a slight variation of the State pattern, where the context is the Controller and each state manipulates the model in different ways using commands from the Command pattern.

The states are also responsible to get the user input needed to perform their logic from the view, by displaying widgets and getting the user selection.

So for now, the Model is just a "box", a stateless container of data without any game-logic in it. We had a discussion with our tutor and he frowned us upon because he said we were deviating from the OOP approach. For example, we have cards in the game which give different effects; in our implementation, we have an "Effect State" which is handling the different effects by controlling their type:

if (effect.type == RESOURCE){
    Command c = new GiveResources(player, effect.resources);
    c.execute();
}else if (effect.type == BONUS_ACTION){
    controller.state = BONUS_STATE;
}...

Whereas he would so something like:

effect.execute(controller);

By passing the controller as a "callback" (forgive me the improper usage) to get inside the effect the input needed to perform its execution.

Now, I understand that our approach is violating in some way the Separations of Concerns principle, by putting game logic in the controller and outside the effects. But, considering that the set of effects is limited (that's why we chose the enum approach), isn't going back and forth between model and controller introducing way too much overhead? Or should the model "own" the game-logic (relating to this question, substituting the boolean with a State field) and call the Controller every time it needs user input?

Answer 1

Your lecturer is teaching you best practices through software design patterns - in this case, The Strategy Pattern (known in simpler languages like C as function pointers). You can follow your lecturer's guidelines AND be elegant / efficient at the same time:

enum EffectType
{
   EXPLODE,
   GAIN_HEALTH,
   LOSE_SCORE
}

public interface Effect //strategy pattern: class encapsulates function pointer
{
    EffectType type; //optional - see hashtable below
    public void execute(object args = null);
}

public class ExplodeEffect implements Effect
{
    public override void execute(object args = null)
    {
        Body body = (Body)args;
        body.Fragment(); //or whatever
    }
}
//...etc. for all types

public class EffectManager //or whatever you call yours
{
    Hashtable<EffectType, Effect> effectsByType = 
                  new Hashtable<EffectType, Effect>();

    void Initialise() //could also do in constructor?
    {
        effectsByType.put(EXPLODE, ExplodeEffect);
        //...etc. for all types
    }

    void RunEffects()
    {
        //for each effect?
        EffectType type = ...; //get the effect enum value / ordinal somewhere
        Effect effect = effectsByType.get(type); //no if / switch block - efficient!
        effect.execute(); //you can also pass args in here if req'd
    }
}

You are not wrong to question bringing in additional classes each just to handle a couple of lines of code for each implementation of Effect. This is one pet hate of mine in languages like Java, where everything is broken down into classes, and functions aren't first-class citizens. In C or JS, I'd just have another function in the same or another module / namespace, instead of having to implement a whole new class like you will have to, just to wrap execute's implementation.

As for the model/controller separation, I have spent a LOT of time on MVC for games and separating model from controller for the same concept (in your case, Effect) is IMO overkill and makes matters nigh unmanageable, especially as a codebase grows. (Then again, I'm the kind of guy that couldn't care less about public, private etc. either.) MC+V architecture is much simpler than M+V+C... but if you do this, you must be very careful about which functions you allow your View to run on the ModelController, since it will need to have a reference to it to read Model state, and could abuse that by calling all kinds of functions that change the state unintentionally.

I would say that in your case, efficiency should not be the primary concern. Writing code in a good architectural form comes first at your level. So listen to and learn from your teacher.