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->...


7

There seems to be a section in your code that you repeat three times. This can probably be placed in a separate function. Something like this: function getInvoiceTotal($status, $onlyOverdue = FALSE) { $query = Capsule::table('tblinvoices'); $query->select(Capsule::raw('SUM(total) as total')) ->where(['userid' => $_SESSION['uid'], ...


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 ...


5

I think you can DRY this up pretty well by refactoring. This feels like a “ifs are evil” code smell to me. There are three “if” statements within that whereHas. They are packed in fairly tightly, and it makes that part of the code more difficult to read. Trying to refactor that as much as possible can resolve the duplication along the way, and ultimately ...


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 of all, you could have less updates if you passed all values you want to set at once: Product::where('id', '=', $key) ->update(['selected' => '0', 'amount' => '0', 'group' => '0']); Secondly, you check in many places in your code if value is different than 0 and save the value, but if later if it is equal to 0 you save 0 in the database - ...


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 ...


4

The main issue I have with your approach is that it is non-standard and purely for the way that you work. Others looking at your code may be a bit confused as to what you are doing and why. In your example, there is (IMHO) little benefit in being able to fold the code as there is not really enough code to need to fold the code up, but in larger code ...


4

I'd like to recommend condensing some of your script so that it is easier on the eyes. This: if($location) { $locations = []; if(is_array($location)) { foreach ($location as $name) { $locations[] = $name; } } else $locations[] = $location; $jobs = $jobs->whereMetaIn("location", $locations); } if($employment) {...


4

You could exploit Laravel collections to achieve what you need. Most of your code is checking for the presence/absence of props on your arrays. Laravel internally exposes and uses the Illuminate\Support\Arr class that contains a lot of useful methods to work with arrays without the hassles of checking for key existance. It also allows you to use dot ...


4

"Premature optimization is the root of all evil." As long as this code works and causes no problem, nothing should be really done. Trying to "improve" it, most likely you will cause a severe harm to readability, maintainability and - ironically - performance. The only thing could be suggested out of the blue is to wrap the loop in a transaction. It will ...


4

I'm mostly a back-end guy who fusses his way through front end code. Here's my advice: Keep the controller code as close as possible to handling the response only. Delegate the creation of objects to their own classes. Think of the S in SOLID - Single Responsibility. public function store(Request $request) { $image = new ImageMeta($base64); $...


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