Design Critique/Review of a God Method for a ViewModel

Question

For some reason the below design doesn't sit well with me, but I'm unable to come up with an articulate reasoning as to why. I would like your help in forming a case against (or for) it.

Say, the design of FooViewModel is as such:

// Some Observables
Observable_1: bool
Observable_2: string;
// --- //

// The method where all the "logic" is dumped
godMethod() {
    if (Singleton1.Status1 != DesiredStatus1) {
        set_state_1();
        return;
    }

    if (Singleton2.Status2 != DesiredStatus2) {
        set_state_2();
        return;
    }

    if (Observable_2.Value != "Desired String") {
        set_state_3();
        return;
    }

    set_state4();
}
// --- //


// In case of ANY external or internal event that may need the observables to change, just call the "godMethod"
externalEvent_Singleton1_Status1_Changed += () => {
    godMethod();
}

externalEvent_Singleton2_Status2_Changed += () => {
    godMethod();
}

Observable_2.Changed += () => {
    godMethod();
}
// --- //

// Actual values of the observables are defined here
set_state_1() {
    Observable_1 = false;
    Observable_2 = "Status 1 is undesirable :(";
}
set_state_2() {
    Observable_1 = false;
    Observable_2 = "Status 2 is undesirable :(";
}
set_state_3() {
    Observable_1 = false;
}
set_state_4() {
    Observable_1 = true;
}
// --- //

I guess this design, instead of defining the logic for each observable, instead defines logic for each "state".

• What are some pros/cons of this design?
• What can be some pitfalls of this design?
• Can this become a maintenance nightmare?

---

My $0.02:

• It is very difficult to find out dependencies for the observables by just reading the code:

  • Say you want to figure out when the Observable_1 is set false.
  • The code would indicate:
    • If Status1 is not appropriate, OR
    • If Status1 is appropriate AND If Status2 is not appropriate, OR
    • If Status1 is appropriate AND If Status2 is appropriate AND Observable_2 is not appropriate
  • Instead of merely defining(declaring?) that if any one of Status1, Status2 or Observable_2 is not appropriate.
  • Do you see how complicated it can become?

• The number of "states" can explode exponentially.

• Calling the godMethod after every action just does my head in; but I'm unable to point to any specific principles that this design violates.

• I think that this may be the "procedural" way of doing things, and it's my OOP brain complaining; but again, why is this objectively inferior/superior?

Finally, just to add, the arguments that I've heard for this design are:

• You can look at all logic at one place, that make the code more readable.
• Whenever any event occurs, just call the godMethod. This, supposedly, makes the code easier to write and maintain.

Answer 1

What you are looking at is the Fix Everything design pattern. The purpose of godMethod is to inspect everything on the page and ensure that state is set to a value that is correct, given the status of the rest of the controls/observables.

The advantages of this approach are:

• godMethod is idemopotent. You can call it any number of times and you'll get the same result, so you can call it whenever you are not sure.
• You only have to write one handler, and all the logic is in one place.
• The approach works well when data can be updated from more than one source.

The cons are:

• A call to godMethod does more work that it would need to do if the code responded to each observable event individually, so performance might not be as good. On the other hand, below a certain threshold, it probably doesn't matter.
• The logic in godMethod may be a bit complicated to work out since you have to look at the page as a whole and account for all possible states.

There is nothing wrong with your "god method" approach, and in fact I think is it better in most cases.

Say you want to figure out when the Observable_1 is set false

I don't understand the problem. If a piece of code doesn't care about state and just wants to know if Observable_1 is false, it just needs to read Observable_1 directly, e.g. var f = Observable_1; What am I missing?

The number of "states" can explode exponentially.

I don't think this is right. If State is a function of the state of the observables, the number of possible values for State is exactly the same regardless of how you approach this problem, as long as you don't add or remove observables.

The only thing that might change is the cyclomatic complexity of the function that computes State. If it has to look at everything, then obviously it will be more complex than something that looks at bits in isolation.

On the other hand, if you did away with the "god" function, the aggregate complexity of having several functions whose combined output is needed to determine State is probably higher still, and much harder to test, as order and repetition may impact the results, and there is no easy means of enforcing sequential coupling unless all the logic is in one place, as it is in your "god method" approach.

Stop overthinking it; your approach is fine.

Answer 2

I do agree with you that the design smells. But I can't come up with specific design that would be better.

What I can suggest is to isolate the switching logic from side-effect code and write complete unit test suite for the switching logic. There are few advantages to that:

1. It reduces the fear you are feeling by giving you some confidence in code's correctness.
2. Properly written test cases will give easier way to infer behavior of the code. So figuring out "when the Observable_1 is set false" will become easier.
3. With proper test suite, it will be trivial to change the internal implementation if you find out better way to implement the design.
4. Properly separating the switching logic and side-effect code might provide cleaner design and improve understanding of internal behavior.

One thing I would be really careful is how to build API between the isolated unit representing the switching behavior and rest of the code. You can either isolate just the godMethod or you can also include the observable events in the testable code. I cannot tell you which option is better with information you given us.

Answer 3

There are a number of things wrong I guess you are thinking about:

• godMethod has multiple exits
• godMethod changes Observable2, which will trigger godMethod, potential infinite loop

The presence of a possible infinite loop shows that the logic for the required state hasn't been thought through. No one is going to specify "when this happens, the app should crash"

Similarly the multiple exits suggest that the logic isn't well defined.

Obviously your code here is just an example, so we don't know the real requirements. Ideally we would be able to distil them down to a single stateless function with a single exit point

state getNextState(status1, status2, observable2)
{
   ...???
   return requiredState
}

state
{
    status1;
    status2;
    observable1;
    observable2;
}

Where the distillation process shows where an input variable isn't used and can be removed, which you could link up in such a way as to avoid infinite loops and be triggered only when appropriate.

You would also expect to see various test methods checking all the combinations of inputs produce the expected output.

The end product might look very similar, but you would be a million times more sure that it was both correct and well thought out.