0

I’d like to merge both of these methods into a single one:

public function addRoute( string $httpMethod, string $route, callable|array $handler ): self
{
    $this->routes[$httpMethod][$route] = $handler;
    return $this;
}

public function addRouteByAttribute( array $controllers )
{
    foreach( $controllers as $controller ) {
        $reflector = new \ReflectionClass($controller);
        foreach($reflector->getMethods() as $method) {
            $attributes = $method -> getAttributes( \Core\Attributes\Router::class, \ReflectionAttribute::IS_INSTANCEOF );
            foreach( $attributes as $attribute ) {
                $route = $attribute -> newInstance();
                $this -> addRoute( $route -> httpMethod, $route -> route, [ $controller, $method -> getName() ] );
            }
        }
    }
}

Bsically the functionality of addRoute() should be inside addRouteByAttribute().

How could I achieve this?

Trece
  • 37
  • 6
Boris Bär
  • 99
  • 7
  • 4
    Why? They do different things, with different inputs. Just call the one you need, or call both if you need both. No point in forcing one method to do the job of two. – Mike 'Pomax' Kamermans Aug 29 '22 at 00:03
  • Do you want to reuse `addRoute` code? – Pedro Amaral Couto Aug 29 '22 at 01:48
  • I don’t think I will reuse addRoute, however I get what @Mike'Pomax'Kamermans says. I wanted to merge them because they actually do the same thing and I can’t think of a good name for both. I like to keep method names clean, so I wanted to have just one method called add but now I have add1 and add2 basically. – Boris Bär Aug 29 '22 at 09:17
  • from the code, you don't have add1 and add2, you have an direct "add" and an indirect "addAfterFindingTheRightDataToDoThatWith" and that's fine. Those _are_ two different things, and having the second method rely on the first is perfectly good code. – Mike 'Pomax' Kamermans Aug 29 '22 at 14:45
  • @Mike'Pomax'Kamermans Yes, you’re right. I’ve came to the conclusion that I leave it as it is for the sake of the SRP. Thanks for the suggestion. – Boris Bär Sep 04 '22 at 13:28

1 Answers1

1

It is not good idea to create one big method which can do many things. It is violation of Single Responsibility principle of SOLID principles. So one method should have one goal.

So if you keep with two methods, your code will be simple, methods will be shorter and it will be easier to edit code and write unit tests.

It is not good idea to merge these methods, but code could look like this:

public function addRouteByAttribute( array $controllers )
{
    foreach( $controllers as $controller ) {
        $reflector = new \ReflectionClass( $controller );
        foreach( $reflector -> getMethods() as $method ) {
            $attributes = $method -> 
                getAttributes( \Core\Attributes\Router::class, 
                               \ReflectionAttribute::IS_INSTANCEOF );
            foreach( $attributes as $attribute ) {
                $route = $attribute -> newInstance();
                $this -> addRoute( 
                    $route -> httpMethod, 
                    $route -> route, 
                    [ $controller, $method -> getName() ] );
                $this -> routes[$route -> httpMethod]
                   [$route -> route] = $method -> getName();
            }
        }
    }
}
StepUp
  • 36,391
  • 15
  • 88
  • 148