Hot answers tagged

9

Since you are using Laravel, I would suggest to use some of the cool features of Laravel: Not necessary, but maybe you want to use the Laravel request feature instead of the $_GET variable: $request->input('id') instead of: $_GET['id'] Laravel input validation is more powerful than you think. Instead of the following if-statement: if (...


9

✔ Using environment variables This is a great way to reduce the risk of credentials being leaked through version control. Kudos. ✘ No PHPDoc comments Although pedantic, your method comments aren't structured to a way that will generate documentation for you (or others). For example, having the following as your comment structure will help both readability ...


7

PHP is not my area of expertise, so just some generic notes: 4 is a magic number here: $post = $this->paginate(4); Why is it 4? What the purpose if this number? A named constant or local variable would be readable with a descriptive name. public function getMonth($post) { $post->month = date('M', strtotime($this->created_at)); $post->...


6

I really like the structure of your code. That said, I almost never suggest to give class variables default values. That is what the constructor is for. For that matter, you don't need to specify that the object's equal null as they already default to that when created. In regards to optional inputs inside of the array, make sure you validate your input. ...


5

Maybe I'm just thinking of a different approach, but I normally would've reversed the dependency between UserEntity and UserRepository. I would expect the UserEntity to represent a singular record in the database and that I would use the UserRepository to retrieve one. In this case, I would inject the UserRepository into the controller and call ...


5

The only problem with this approach is that as you add more methods, you are going to end up with an awful lot of repeated if (Request::ajax()) ... blocks all over your code. Here's what I do for my sites: <?php class ApiController extends Controller { protected function makeResponse($data, $isError = false, $redirectToIfNotAjax = '/') { if (...


5

Security Try to follow current best practices. If you are very new to web security, take a quick look at the OWASP Top 10. SQL Injection You are open to SQL Injection via the $urlid parameter as well as $browseragent, $reqpath, and possibly $ip and $host. As well as do second order injection via $username and $projectName. And that's just in your first ...


5

DB Manager thoughts: 1) I am generally not understanding what you are trying to achieve by even having this class. It is not doing anything like managing database connections, abstracting the user from SQL formulation, adding transactional handling around queries, or other things that one might often implement a class such as this to do. As it stands now, ...


5

Style guide I'd recommend picking a style guide to keep all of your whitespace consistent. I've formatted yours according to semistandard. Having a style guide could be considered a form of bike-shedding, but it lets you have your IDE focus on making your code readable while you can just focus on making it work. Try to limit the use of jQuery You're using ...


5

Security: Credentials in ENV It is less than ideal to store your credentials in environment variables, as they may easily leak. phpinfo for example will print all environment variables. Of course, you don't want to allow access to phpinfo to just anyone either, but it may still leak, and not everyone who should be allowed to see phpinfo should be allowed ...


4

I didn't used PHP in the last few years, so just some generic notes: The $input parameter of insertUserIntoDatabase($input) is unused. I guess you wanted to use the parameter instead of direct Input:: calls inside the function. You could use a guard clause in insertUserIntoDatabase to make the code flatten: public function insertUserIntoDatabase($input) { ...


4

You should reconsider the responsibilities of class UserServiceProvider, too many responsibilities may lead to a lot of dependencies in many cases. It is quite possible that this class is doing a lot of work that it should not, for example populating User object with sales reports, earnings reports, or activating user, checking activation status etc. It ...


4

First I'd like to start off by saying that if that code suits you, then it's not a bad idea to stick with it. It's fairly clean and pretty neat. I'm just going to point out a few things that may diverge from different types of implementation. Let's talk about the store and update methods. You are injecting a Reqeust class in it, which means your repository ...


4

private function webbugRootExecution($urlId,$strLocation) { $db = new DBManager(); if(!empty($_SERVER['REMOTE_ADDR'])) { $ip = $_SERVER['REMOTE_ADDR']; $host = gethostbyaddr($_SERVER['REMOTE_ADDR']); $reqpath = $_SERVER['REQUEST_URI']; $projectID = substr($reqpath, $strLocation); $projectID = ltrim($projectID, '...


4

From a short review: I hope your id's are not simply an increasing number, otherwise this: var data = 'deleteIds=' + JSON.stringify(deleteIds) + '&_token=' + pageToken; is just asking a hacker to go to console and delete every single item. To prevent that you should use GUIDs. As a very high level comment, check out MVC. You can tell you need ...


4

There are some major concerns with this approach. First, why rebuild all the DOM elements every time there is a change? Your universe of different countries, provinces, districts, etc. is static right? You could either render all DOM elements needed for the page up front (probably the simplest approach), or if you find this is too much bandwidth to ...


4

I honestly don't think there is much to "clean up" here, as this seems pretty straight forward. A few stylistic notes: It is pretty standard in PHP to have variables named with first letter in lower case. So consider $UploadedFile => $uploadedFile. Why blank line between most every line of code? This seems excessive. Vertical white space can be good to ...


4

Code intent The main problem with your code is that it does not give any reasons as to why it performs the way it does which makes understanding, reviewing or upgrading it difficult. $event->body->id What is a body id on the event? is it always present? I'm assuming its an int but I guess it could contain letters? Does $event_id adequately describe ...


4

First of all do not use Laravel 5.4 even if you've written some* code already, use latest 5.7, and try to always update. *: You really need to rewrite the application if you have code like you presented everywhere, so just take your time and use L5.7. Idea is already there you just need to improve it - pure coding, no thinking. Model(s) Standards You are ...


4

You could use route model binding to remove some lines from your code, but that would require changing the route so it passes the MenuItem's id. public function update(UpdateMenuItemRequest $request) public function update(MenuItem $item, UpdateMenuItemRequest $request) { $validated = $request->validated(); $item = MenuItem::findOrFail($validated['...


4

I'm sorry, this is not an answer to your question, but it is a partial review of your code. It seems you've got a serious problem with choosing your names. They don't actually tell me what the purpose of the methods is, or the content of the variables. Your method getDocument() can do two things: Return the path or the folder of the document. The name ...


3

There is nothing wrong with this approach at all. Rather than say you should or shouldn't do it any other way I'm going to give you an alternative approach that allows you to write less code in your routes.php file. I've noticed that your HomeController deals with a lot of different types of requests. This can get very hard to manage when your application ...


3

I am not an expert, but what I am doing is to try to maintain Single Class Responsibility in my project. Basically organize your project like this: Create controllers as you wish (without logic) (no business logic here) Create eloquent models as your database schema is (no business logic here) Create repositories for all models Create small services that ...


3

First of all, I really don't like to mix up routes of two different domains/apps in the same routes.php or web.php. So, I prefer and would suggest you as well to follow the below approach, which seems much cleaner and much authentic. :) Also, I believe www.abc.com or www.abc.dev are not required to be mentioned here... Laravel is intelligent enough to work ...


3

Encapsulation is such an old term from the C++ days. Nowadays it's more SOLID principles that you should build your PHP code on. I'm going to say it in 2 different ways. Yes, it takes a bit of sense of Encapsulation. However, it's very messy as an example. The best example would have been better with a student with its properties and a simple index.php ...


3

SQL formatting Your SQL script in my opinion could use some indentation and a few more line breaks. Variable names Table & column aliases are useful. But it is a good practice to use a variable name that at least says something about what the table contains. Consistency SQL is not case sensitive, but still it is good practice to use one style and ...


3

Seeing I was asked to post my comment as an answer, I've set about typing this little thing up. It's, essentially, a slightly more verbose version of my initial comment. Well, all in all, a controller action is a method, so the same "rules" should apply: 1 type of request goes to 1 distinct action method. Your method seems to be dealing with 3 distinct ...


3

"I want to know whether the created controller matches the standards." I'm afraid it doesn't. Laravel's website states (on its home page): Love beautiful code? We do too. Your controller is really quite messy, to be honest. What is a controller? As the name suggests, a controller is the component that controls how a request is handled, what components ...


3

I would recommend to go the OOP way and create a class which has all those fields as members. Then you do the initialization once and let the member functions just use the fields. This nicely avoids the code duplication and opens up opportunities for better design in general. Also the local variable $pickup_line1 doesn't seem to be used in store() is this ...


Only top voted, non community-wiki answers of a minimum length are eligible