Is often using int constants as parameters in communication between objects considered bad design?

Question

An app I'm working on is designed with MVC. The components often interacts with each other by passing int constants, which then have to be interpreted with if statements. I'm wondering if this is considered bad OO design. (Not specifically with MVC, but in general).

For example:

When the user clicks a specific button, the controller is told by the view controller.somethingHappened(Controller.SOME_INT_VALUE);

The view uses a constant int value in the controller as a message to tell what happened. The controller then has to have a lot of if statements to interpret the message from the view. E.g.:

public void somethingHappened(int message){
 if(message == AN_INT_VALUE) doSomething();
 if(message == ANOTHER_INT_VALUE) doSomeThingElse();
 if(message == A_DIFFERENT_INT_VALUE) doAnotherThing();
}

As a result the controller passes another int value into the model, as a message to do something. I.e. model.doSomething(Model.SOME_INT_VALUE);.

Which forces to model to also have a bunch of if statements:

public void doSomething(int message){
 if(message == AN_INT_VALUE) doSomething();
 if(message == ANOTHER_INT_VALUE) doSomeThingElse();
 if(message == A_DIFFERENT_INT_VALUE) doAnotherThing();
}

This results in both the controller and model having a lot of ifs to interpret the messages the receive.

Is this considered ugly and/or bad design? Should I try to pass actual objects into methods, or make more specific method calls - instead of passing constants that have to be interpreted with a bunch of ifs? Or is this practice of using a lot of constants as messages considered reasonable?

If it is considered bad design, how can I avoid this?

(Please note: For the purpose of this question I'm also referring to enums).

• Rémi: Why those aren't enum? At least the information would be centralised
• old_timer: centralized doesnt mean or even imply synchronized. enums can be far more of a headache than fixed numbers, but you have to do your system engineering to determine which is better
• James McLeod: You could also have an array or a map of functions indexed by the message type, and only check to see if message type has a function to call.
• Dunk: In general, the best approach is to simply call the method on your controller directly. There's no point in encoding your operation to an int, pass the encoding to the controller, have the controller decode the int to an operation and then call the operation. Instead, simply call the operation directly and bypass all the int encode/decode stuff entirely. Even though you are using if's, what you really have is a BASS and those are always bad and 99% of the time there's a better/cleaner way to accomplish the same thing.
• user22815: @dwelch using primitives such as int is wrong for two reasons. First, you are not performing math on them: this is the same reason why a telephone number should be encoded as a string, not an int. Second, you can use them out of context. If you define colors as ints, you could pass a color to a function requiring a shape. If you instead use an enum or class, that will no longer work (at least not in languages with first-class enums, i.e. not C/C++) which is a good thing.

Answer 1

Is this considered ugly and/or bad design?

Yes.

Should I [...] make more specific method calls - instead of passing constants that have to be interpreted with a bunch of ifs?

Yes, that's exactly what you should do. Unless there is some reason not to. Given that you've already thought of this much cleaner option yourself, I suspect that there might be, but it's not discernible from your question.

Update: If the called methods would have duplicate functionality, the object oriented solution is to put the functionality that differs into separate classes that all implement the same interface, and call that from within the function, using polymorphism instead of explicit branching.

So, using the information from the comment, you'd call

controller.applyStyle(new BoldStyle());

or

controller.applyStyle(new UnderlineStyle());

where BoldStyle and UnderlineStyle both are classes that implement a Style interface (or inherit from the same superclass) which contains one or more methods that do the different things and which are called by the applyStyle() method.

• Aviv Cohn: Here's why I used constants: I'm working on a word processor. There are three buttons: Bold, Italic and Underline. When the user presses one of them the view has to report the button press to the controller. This can be done by having the methods setBold(), setItalic() and setUnderline() in the controller which is fine. But in my situation all three of these methods do almost the same thing and only vary in one or two lines. In this case, having three different methods results in a lot of code duplication. So in a case like this should I pass constants instead of separate method calls?
• Michael Borgwardt: @Prog: see update.
• Aviv Cohn: I see. So basically applyStyle() would have some logic, and at some point in it's code would call style.execute() (or similar) - which is the point in the code where the required logic varies from style to style (style being the Style object passed in as a parameter). Or maybe it would delegate the Style to the model and the model would use it for it's functionality. Did I understand correctly?
• Michael Borgwardt: @Prog: yes, exactly. Ideally, you'd find that there are other places where you need style-related behaviour or data, and can collect all this in the Style classes.
• Michael Borgwardt: @Prog: Yes, I would consider it OK, for pragmatic reasons. I don't think it's useful to be dogmatic about this. The point of decoupling in MVC is to improve maintainability by separating concerns. As long as the view's "touch" of the business logic is restricted to choosing which aspect of it to call, this separation is achieved.

Answer 2

There are several choices here, in increasing order of preference (most preferable last):

• Bits (flags) in an integer. Can be confusing, but allows packing a lot of them into a single int. So

  DO_THIS = 1 DO_THAT = 2 DO_ANOTHER = 4 DO_ZING = 5

  perform(DO_THIS | DO_THAT)

  if (command && DO_THIS{ doThis(); } ...etc

• Integer commands

  DO_THIS = 1 DO_THAT = 2 DO_ANOTHER = 3

  perform(DO_THIS, DO_THAT)

  for (command in commands): if (command == DO_THIS) { doThis(); }

• I favor strings over ints, just because they're more readable

  DO_THIS = "this" DO_THAT = "that" ...

• "Typesafe" constants (in case you don't have enums around)

  DO_THIS = new Object(); DO_THAT = new Object();

  perform(DO_THIS, DO_THAT);

  for (command in commands) if (command == DO_THIS) { doThis(); }

• Enum is a formalization of "Typesafe constants" so use that if you have them

...

Generally, though, these schemes are useful when the calls have to be serialized & transmitted between processes. If you're just making calls from one section of code into another, you can just identify what you want done with the function names. Writing some sort of "router" function that makes a call to lower-level function replicates what the language does for you, so you might think harder about whether you need that.

...

That is, I try to keep it so that 90% of my code looks like it came straight out of "Learn Java in 30 days" (or Python, or whatever). That is, the code should just use the language in a really basic way almost all the time. There should be a pretty good reason why you can't just do it the basic way before you move to a broader generalization.

• user22815: The argument for using objects instead of direct function calls is one can decouple separate areas of the code and pass events instead. That means that e.g. the view does not need intimate knowledge of the controller (asker did mention MVC). An event could have a topic that is a string or enum describing what caused the event, which is strongly typed and meaningful.
• Aviv Cohn: @JohnGaughan Is the Command pattern an example of what you describe?
• sea-rob: Command is a little different. It's a little hard bending this example to suit it. Essentially, you'd have to instantiate a SetStyle object that had a single run() method, and have the target of the style as an attribute. Then the MVC code would just call setStyle.run() to get the job done. But in this case, that's going the LONG way around. Command is really powerful, but I mainly use it in asynchronous processing, like shoving tasks into a work queue.