Skip to main content ↑ ↓
Topic: Topic and MoveTopic (and MergeTopic) controllers (Read 6488 times) previous topic - next topic
0 Members and 1 Guest are viewing this topic.

Topic and MoveTopic (and MergeTopic) controllers

Yesterday evening I was looking at the action array and I was wondering why the MoveTopic controller was not into the Topic controller with a sub-action action_move and so:
https://github.com/emanuele45/Dialogo/compare/master...movetopic
Then I realized... why not MergeTopic too?
That one has a bit more ramifications, but should still be doable.

Does it make sense to do move that (sub)actions to Topic?
Bugs creator.
Features destroyer.
Template killer.

Re: Topic and MoveTopic (and MergeTopic) controllers

Reply #1 –

I think the reason why it wasn't done is because it makes the controller a lot bigger. I would say if it does make it too big, call it something like "TopicExtra" or "TopicAdmin" or "TopicModeration" and combine all of the moderation stuff in there.

Re: Topic and MoveTopic (and MergeTopic) controllers

Reply #2 –

I suspected, the real reason was another, but I can't be sure and that may sound a good one too: topic with all sub-actions (move, merge, remove) should be about 1800 lines (roughly, but the Merge and Remove should become shorter due to some queries and things that have to be moved out, so let's say about 1600).

The problem with Topic controller is that there isn't much more about topics than moderation: the three actions currently in that controller are: lock, sticky and printpage.
So, in my mind, I would better see printpage as "sub-action" of Display and have all the moderation-related stuff in Topic.

And what about using Topic as a sort of "second dispatcher" to do some actions or call other controllers?
In fact a sort of collection of wrappers for the most tricky actions like move, remove and merge:
Code: [Select]
public function action_move()
{
    require_once(CONTROLLERSDIR . '/MoveTopic.controller.php');
    $move = new MoveTopic_Controller();
    return $move->action_move();
}
and so on?
Bugs creator.
Features destroyer.
Template killer.

Re: Topic and MoveTopic (and MergeTopic) controllers

Reply #3 –

I agree with moving printpage to the display. Don't agree with using Topic to redirect to the other controllers. That would make it harder to find what you're looking for. Lessen code duplication by loading Topics.subs.php there and checking "if (empty($topic))" (not by much). Even better would be to move the call to the function that loads $topic to pre_dispatch().

Re: Topic and MoveTopic (and MergeTopic) controllers

Reply #4 –

Recently in the development branch I reduced the Merge and Move controllers to "nothing" (~300 lines each), now the sum of the three would be something like 700 lines of code, I guess that now it may make sense to have them all in one single controller. O:-)
Bugs creator.
Features destroyer.
Template killer.
 

Re: Topic and MoveTopic (and MergeTopic) controllers

Reply #6 –

Yep!
Bugs creator.
Features destroyer.
Template killer.