New answers tagged php
1
This review will focus on the PHP to generate the queries. Other reviewers may have suggestions about improving the partition process.
General feedback
The code is easy to read and mostly is in line with the recommendations in the PSR-12 style guide. The methods have good docblocks to describe the purpose and list parameters, return values and possible ...
0
Thank you both for helping me out. I have upvoted both of your answers.
This is what I have made and it works great so far, I would still need to test every API call just to be sure.
if(empty($_GET['action'])){
echo Display::json(400);
return;
}
$argumentNames = [
'getInfo' => [],
'getToken' => ['PHP_AUTH_USER', '...
3
As Kiko pointed out, there are many repeated blocks to check if the user has sent too many requests for the given action - e.g.
if(Database::userSentToManyRequests('getToken')){
echo Display::json(429);
return;
}
This violates the Don't Repeat Yourself principle . That block can be moved out of the switch statement - e.g.
$action = $_GET['action'];
...
3
I'm quite surprised that you placed the userSentToManyRequests() function in every case. If you do that then you can just start with it before the switch ().
To get rid of the switch you can use an array containing the names of the arguments for each action function. I've done it for the first four cases, but it is easy to do them all:
$argumentNames = ['...
0
<?php
// It might be good to write functions to describe at least what is happening by their name
$file = "countries.txt"; // fopen is only safe when using this constant, don't accept user input as an argument
$file_handle = @fopen($file, "r"); // don't use @ to suppres errors, it will make debugging so much harder https://...
2
The way I see it, your whole process can be boiled down to this:
<label for="country">** Select Country: </label>
<?php
$countries = file('countries.txt', FILE_IGNORE_NEW_LINES | FILE_SKIP_EMPTY_LINES);
if (!$countries) {
echo '<input type="text" name="country" id="country" maxlength="40"...
2
Unless it is preferable to use fopen(), fgets(), fclose(), etc. - e.g. for the sake of performance, it could be simpler to use file_get_contents() to read all of the lines into memory and loop over the lines.
It is extremely rare to see goto within PHP code. Idiomatic PHP code would simply use conditional blocks. Even the PHP documentation for goto contains ...
3
Consider using concatenation:
rather than $label_input_html = $label_input_html . ...
use $label_input_html .= ...
You're outputting HTML, so end $label_input_html with <br>, not \n.
Also see the "\t", so if are you doing all that for display purposes, like to show your work, that's fine.
I don't understand the END:. AFAIA that's completely ...
1
While there isn't a lot of code here, there is some duplicated code - e.g.
$extraParams = ['tenant' => $tenant->api_tenant];
$uri = $request->getUri();
$uri .= (isset(parse_url($uri)['query']) ? '&' : '?');
$uri .= http_build_query($extraParams);
in the callback to Middleware::mapRequest passed to $handler->push() in the factory() method, ...
1
The reset mechanism can probably be simplified. Instead of making it a submission button within your form, you can enjoy the same effect, by creating an <a href="">Reset</a> tag and styling identically to the other submit button. This way, when Reset is clicked, there will be no POST request and you can safely omit the 4 lines of code ...
0
Going through your code, it looks like a really good implementation, although when using bramus you could handle the method directly from the route, example:
$router->get('/listBooks','BookController@showall');
7
A few other more minor bits not mentioned by Dharman, but agree 100% that the code is very good to start with, the Post - Redirect - Get pattern stands out many login systems do not have that implemented well.
Consider 2fa
There are plenty of packages which implement totp or u2f use one of them for an additional security boost
Structure of file system
It's ...
4
Sam's answer covers a lot of good points I am not going to repeat, but I will tell you more issues.
Private properties
What is the point in setting constants as private properties? What is the purpose of having all these config values as private properties if they are only used in the constructor? These values should come from a config file that is stored ...
18
This is a pretty good attempt at a login system. The code looks much better than 99% of code that I see on Stack Overflow. Keep up the good work!
Your code is almost clean. Your classes do so little that I would consider it too little (more on this later). The naming is almost perfect IMHO. Nonetheless, I have noticed a number of smaller issues that I think ...
3
Your package builds on the getBlocks() method of EditorJS. You get all blocks, which represent HTML tags and their content, and you render it as HTML output using PHP template files. The ugly code is probably hidden in these templates?
You have not explained why you would want to access the content of a block using a stringed key. What is the practical use ...
3
Canonical advice from a helpful post
As was mentioned in a comment there is a post with helpful advice for classes like this: Your first database wrapper's childhood diseases (written by @YourCommonSense). It recommends setting the exception mode to PDO::ERRMODE_EXCEPTION which appears to be the case with the code here. It also recommends:
Tell PHP to ...
0
Imagine, instead of having a long list of code within the block file, to read:
setDefaultImage()
setAlignment()
setPadding()
//...
As a reviewer, I would see the key parts of what is happening without having to think about specific code. Should I want to know what specific function does, I can click into it.
The long list of variables can also be ...
Top 50 recent answers are included