Hello everybody,
Today I was busy writing some code.
My question is could I improve my code ? ( even a little bit )
Or is it OK for now.
Here is the link to the gist.
https://gist.github.com/concept-core/b2ae6a020bd4d4d896e46ae096c02710
1 Like
It would help to have some context. Is this a framework you’re building? Or is it an application within an existing framework? If so, which framework?
https://gist.github.com/concept-core/b2ae6a020bd4d4d896e46ae096c02710#file-cmscontroller-php-L10
It looks like your application controller and the framework base controller are in the same namespace. Prefer instead to have separate namespaces for the framework and any application that uses the framework.
https://gist.github.com/concept-core/b2ae6a020bd4d4d896e46ae096c02710#file-cmscontroller-php-L28
Is this config function global? Prefer passing config data as arguments (aka dependency injection).
https://gist.github.com/concept-core/b2ae6a020bd4d4d896e46ae096c02710#file-cmscontroller-php-L53
Prefer to avoid special case logic, such as abort. If the controller’s job is to return an HTTP response, then you can still do so. Just return a status 404 response.
https://gist.github.com/concept-core/b2ae6a020bd4d4d896e46ae096c02710#file-cmscontroller-php-L57
It may not be a good idea to have the model defined in the route, but it’s hard to say without knowing more about this framework. Also, rather than assigning the model value to $this, prefer instead to simply pass it as an argument to getPageInfo.
https://gist.github.com/concept-core/b2ae6a020bd4d4d896e46ae096c02710#file-cmscontroller-php-L70
Prefer dependency injection over new-ing up your own objects.
https://gist.github.com/concept-core/b2ae6a020bd4d4d896e46ae096c02710#file-cmscontroller-php-L85
Do you have multiple values serialized in your database? Prefer to use the database’s own mechanisms for handling multiple values (rows, columns, joins, etc).
Also, this value doesn’t need to be assigned to $this either. You can just return it. getPageInfo can accept the model name as an argument and return the page into as a return value.
https://gist.github.com/concept-core/b2ae6a020bd4d4d896e46ae096c02710#file-cmscontroller-php-L63
I have no idea what this view function does or returns (a response object? a string?). Though, it seems to be global. Prefer dependency injection.
3 Likes
From the naming of methods and classes this looks like Laravel.
BTW, all very valid points!
2 Likes
Ahh, yes. Looks like you’re right. It also appears that several of the things I criticized is just how Laravel does things. The global config function, for example. And the global view function. And the global abort function. All of which, in order to work, rely on there being a single, globally accessible app.
Bleh.
1 Like
You can also use dependency injection avoiding facades all together. It’s not really how Laravel does things like you said but facades and a few helpers are provided for convenience. If you don’t understand more advanced design patterns than those things make the framework easier to work with. For people newer to programming it decreases the learning curve. Though in many cases you can argue using facades in controllers is fine because controllers are typically tightly coupled to an application anyway. Also facades are fronted by class instances that implement a contract/interface. So it’s really not all that bad.
1 Like
Laravel seems to be a framework to get things done easily rather than follow the best coding practices. Although, I believe that a skilled programmer can follow best practices with Laravel, however such practices are not always enforced by the framework.
1 Like
First off, Thank you for your replies.
It is my frontend controller for my laravel project.
I want to make a small CMS for my own website that checks if the url exists in the database.
If it exists in my routes table it is gonna check if it exists in the appropriate table that is linked to the model.
Also I am changing some stuff up as we speak, but my pages table holds a json string with information about the page. ( multiple editable fields with content, meta etc. )
After that I am checking if the page/user/blog post exists in that table to. ( because you never know what bug appears in development )
The config, abort en view function are part of laravel which is why I use them.
Still trying do some with the constructive criticism, but I am not going to use as much best practices since some of them are not needed because you can still read the code properly and it works the same.
I disagree.
This is the config function.
function config($key = null, $default = null)
{
if (is_null($key)) {
return app('config');
}
if (is_array($key)) {
return app('config')->set($key);
}
return app('config')->get($key, $default);
}
This is the app function.
function app($make = null, $parameters = [])
{
if (is_null($make)) {
return Container::getInstance();
}
return Container::getInstance()->make($make, $parameters);
}
Therefore, you can just as easily inject the config into the controller like this.
use Illuminate\Contracts\Config\Repository as Config;
class MyController {
public function __construct(Config $config) {
}
}
All frameworks provide some helpers and Laravel is no exception but you can also do things the *proper/long winded way as well.
Well, it depends on what you value most in your code. If you value terseness then this facade-style shortcuts are a good solution. A few years ago I would have liked them a lot because then to me the shorter the code the better and simpler it appeared. Now my values have changed and to me clarity and obviousness is more important than short code. Of course, code should be as short as possible but not at the expense of clarity. If the actual method is hidden behind a magic static call and I can’t get to it immediately by control-clicking it in my IDE then I consider it too much magic - unless there’s a really good reason for it. In this case there isn’t - just saving a few keystrokes is not good enough benefit.
All frameworks provide helpers but they don’t have to be hidden behind magic. I’ve found that the long winded way pays in the long run and saves time - and it’s really not that long winded.
1 Like
Well lemon.
I agree with you a 100% but I am not yet at the point that every thingy should be super obviously
Although when I actually work in a company where they guide me a bit better I do want to be really obvious.
But for now it should work and be quite clear what it does.
And I mean that in a sense where I don’t want to be a jerk or want to be smart.
I just don’t see the value in it to be super clear.
Also I was looking for actual things that should be better because you cannot figure out what it does.
Best regards.
If your ide doesn’t support Laravel finding the class is really quite simple with facades. You just need to go look at the facade class and in there will be the name of ioc binding. Pretty clear to me. I think you’re making it out to more complex than it really it is. It’s really not all that complex and Laravel itself is pretty easy to walk through. Especially using a smart debugger.
Of course, and I didn’t mean to tell you you should do it this way or that way but since you asked for what can be improved then you may expect different opinions on that matter - sometimes even conflicting ones.
I’m just saying that over time I’ve come to appreciate clear and non-magical code - which is especially beneficial when a different programmer that knows nothing about your code tries to find his way around it. I’ve had the opportunity to try to make some changes to web sites based on frameworks foreign to me and what made the task most difficult was the magic: like there was a call Class::method() and there was no method() anywhere to be found - after some long digging around it turned out that Class::method() actually calls Class::nicelyNamedMethod(). It really helps in debugging when everything is clear and obvious and type-hinted.
Having said that - we use what needs to get the job done and often we can’t apply the best theories everywhere. Especially in smaller projects they don’t matter so much. And if you are using Laravel even without conforming to the best practices (if such exist) - it’s still an order of magnitude better than using no framework spaghetti code that is so popular in PHP.
1 Like
I know facade classes add little complexity. But what benefit do they bring? What problem do they solve?
I don’t think this is much a concern with smart debuggers. Anyone can easily place break-points in code and walk through it. If you’re not using a smart debugger than that’s your fault and you have no place to complain about “readability” of code.
So the only benefit is “terse and expressive” syntax? And the price is getting rid of the clarity that dependency injection provides and having to use a debugger to follow the code? Considering the benefit is so small and inconsequential compared to the cost that I’d choose plain DI any day. It’s true that the underlying complexity of facades is not really big but it’s still bigger than the benefits and I prefer to use debuggers for more serious stuff than figuring out what a magic static call does. Personally, this feature looks to me like bloat because it doesn’t provide any substantial benefit to justify the existence of the additional framework layer needed to support it, however I understand some people may like it. We’ll have to agree to disagree!
You don’t have to use a debugger. Although I don’t really understand why you wouldn’t want to. i use xdebug. To each their own.
I never said I don’t use a debugger, of course I do. But it’s much faster to control-click or control-B on a method than firing a debugger. I really appreciate frameworks which are totally non-magical, I just control-click on any method call and I instantly go there and step by step I can very easily find my way in foreign code without reading any docs. The debugger is always there for more serious stuff.
I don’t really follow you. I use xdebug via phpstorm for just about everything. If I need to know what a facade is doing I just simply place a break-point and step through the code.
Surely not having to control-click or control-B is quicker that having to press keys.
With xdebug the debugger is always running to report i detail on errors. You only have to go into the debugger if you need to set breakpoints.
Than pressing what keys? Control-click or control-B takes me to the method definition in less than a second.
Can you go to the debugger, set the breakpoints and navigate with it to a method definition in less than a second? I would like to learn how to do that.
What I wrote above. Simply a matter of speed. Perhaps I need to learn how to use xdebug at sub-second speeds?