Skip to main content
9 votes
Accepted

Phishing Project Assessment

✔ 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 ...
ʰᵈˑ's user avatar
  • 336
9 votes

Sanitize URL string for Insertion Upon DB table via Eloquent model

Beware of re-assigning a variable. In the setter method setUrlAttribute() the variable $saneUrl is over-written numerous times: ...
Sᴀᴍ Onᴇᴌᴀ's user avatar
7 votes

Queries to get unpaid and unpaid invoices

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: ...
KIKO Software's user avatar
7 votes
Accepted

Setting default of <options> in Laravel with PHP Match

Some variable assignments are excessive Let's look at what $selected is set to: $selected = 'selected'; Then in both of the <...
Sᴀᴍ Onᴇᴌᴀ's user avatar
6 votes
Accepted

A Search Engine Class

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 ...
Trojan404's user avatar
  • 818
6 votes

Sanitize URL string for Insertion Upon DB table via Eloquent model

As some general feedback, I would say that your unit tests are lacking. It looks like you are just checking that your filter function modifies the input in some way? One of the main jobs of tests are ...
Pierre Menard's user avatar
6 votes
Accepted

Sanitize URL string for Insertion Upon DB table via Eloquent model

The biggest problem here is that you are going overboard with precautions. Security is important, but it must be logical. What ...
Your Common Sense's user avatar
5 votes
Accepted

Simple inbox functionality using JavaScript and Ajax

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-...
Dan's user avatar
  • 3,748
5 votes
Accepted

Phishing Project Error Logging

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 ...
Mike Brant's user avatar
  • 9,638
5 votes
Accepted

Phishing application God class

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 ...
tim's user avatar
  • 25.1k
5 votes
Accepted

Document Merger using PhpDocX

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 ...
tim's user avatar
  • 25.1k
5 votes

Laravel 5.8: prevent duplicated code

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 ...
James Clark's user avatar
5 votes
Accepted

How to reduce a POST request duplicate in Laravel/PHP for payment system?

Let's implements following: separate logic, refactoring of methods Separate logic Validate Requests/PaymentRequest.php add rule for validation ...
Kostiantyn Okhotnyk's user avatar
5 votes
Accepted

Update specific model attributes from a request

How does this look? It looks okay. It appears to mostly adhere to PSR-12 and is easy to read. While there is only one method with a single loop it isn't very complicated. The variable names are ...
Sᴀᴍ Onᴇᴌᴀ's user avatar
4 votes
Accepted

Authorizing users for POST requests

I do not see much thing to review, except goto. Code style... ...is OK. I will suggest functions to be no more than 10-15 lines, ...
Nick's user avatar
  • 1,582
4 votes

Phishing Project Assessment

...
Pimgd's user avatar
  • 22.3k
4 votes

Simple inbox functionality using JavaScript and Ajax

From a short review: I hope your id's are not simply an increasing number, otherwise this: ...
konijn's user avatar
  • 33.2k
4 votes

Populating a form with filtered hierarchical place names

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 ...
Mike Brant's user avatar
  • 9,638
4 votes

User Avatar upload

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 ...
Mike Brant's user avatar
  • 9,638
4 votes
Accepted

Automated Tweet of new inbound Enquiries in PHP7 (Laravel 5.2)

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. ...
arcanine's user avatar
  • 364
4 votes
Accepted

Laravel UserController Design

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 ...
Kyslik's user avatar
  • 181
4 votes
Accepted

A PHP Controller, which feels to 'array-like', in Laravel

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. ...
IGP's user avatar
  • 158
4 votes
Accepted

A method to get a document's path

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 ...
KIKO Software's user avatar
4 votes
Accepted

Using PHP goto Labels for code folding

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 ...
Nigel Ren's user avatar
  • 368
4 votes
Accepted

Laravel controller method that searches jobs using variable criteria

I'd like to recommend condensing some of your script so that it is easier on the eyes. This: ...
mickmackusa's user avatar
  • 8,674
4 votes

Refactor PHP | Laravel Code, Optimisation needed to make code clean

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 ...
mdexp's user avatar
  • 141
4 votes
Accepted

What would be the best way to update data from associative array?

"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 ...
Your Common Sense's user avatar
4 votes

Structuring code logic for events on laravel controller

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 ...
Nick's user avatar
  • 141
4 votes

Laravel controller method to update user

So the above answer from Sᴀᴍ Onᴇᴌᴀ already covers quite a few points. I've worked with Laravel for the past 4 years, so I'll go a bit further in depth on how I'd approach this, including a few ...
Tropus's user avatar
  • 61
4 votes

Controller method to store course

You could create 2 sub-functions for the different parts in your function. validate($request) so that long part is hidden ...
Double_A's user avatar

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