Hot answers tagged

10

Security Security is super important. In 2020 it is not acceptable to write code that is blatantly vulnerable to SQL injection. Start taking good habits today. Even if you are lazy and you are the only one using this application you should still fix the SQL injection vulnerabilities because your code will choke on single quotes - and single quotes in family ...


10

Your project looks nice, and the design of your web page is good. There were a few points I spotted with the code that are well worth taking note of. Security While security may sound less important if you're the only one using the project, the issues below are both critical for any public-facing website, and affect the correctness of the program as pointed ...


7

I've noticed that you are using Bootstrap 3.3.6, which isn't maintained anymore. The latest 3.x version is 3.4.1, and you should consider using it. However, you're doing something wrong: You are misusing the framework Below is a copy part of the HTML, with all the CSS: body { font-family: Ubuntu Mono; background-color: #F2F1F0; } .form-control {...


7

You have turned simple SQL prepared statements into this monstrosity that is extremely difficult to read and is open to SQL injection. Remove it! Don't go down this path. It is absolutely unnecessary. Take a look at how simple prepared statements can be. <?php mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT); $mysqli = new mysqli('localhost', '...


5

I'll focus on SalesRepresentatives class, because that seems to be your main concern. Documentation There is none. Note that "Get TargetLevel" for a function named getTargetLevel() doesn't add anything. Getter/Setter Asymmetry I'd expect getTargetLevel() to give me what I previously set using setTargetLevel(). That is very surprising behaviour. ...


4

Creating your list of decks """-- ORIGINAL CODE --""" #I'm not entirely sure how this works #Somehow it takes everything with the same deckId (index:2) and puts them into one list values = set(map(lambda x:x[2], decks)) newlist = [[y[1] for y in decks if y[2] == x] for x in values] map() takes the function in the first argument,...


4

Your code is good enough at preventing sql injection. But it sucks (pardon me) in everything else. Don't print database error to the user. Don't put echoes into your logic. Don't double last letters of post fields names. Don't repeat yourself by integrating your database connection to every class that has nothing to do with db. Prefer composition. User ...


4

In short, no. Actually doesn't seem to be structured in any way :D If your API will be just that, you can even give up on slim and have a plain index.php with url rewrite so you can achieve the best performance. Few things that I recommend: first, I would try to use a DI (dependency injection). I like php-di ( https://php-di.org/ ) next would be to try to ...


4

Does this appear as proper use of the commit and rollback functions? I honestly haven't used PDO functions directly - typically used a framework like Laravel or similar. I do agree with the advice on phpdelusions.net and it has a page about PDO usage with a section about Transactions. It mentions: Please note the following important things: PDO error ...


4

Retrieving the ID of newly-created user should not be done like this: $select = $dbc->prepare("SELECT MAX(uid)+1 AS 'NEWUID' FROM mainTable"); Instead use PDO::lastInsertId() But you are not using variable $addnewuid. So you are doing one query for nothing. Is your code complete and working as expected ? At the moment there is no firm relation ...


3

By improvement, I suppose you have performance in mind because your queries are fairly basic and not hard to understand. I have had a look at your SQL fiddle and your tables have no indexes. Absent any other optimization mechanism the database engine will have to perform a full table scan to fetch the wanted rows. Thus, I don't expect much difference between ...


3

Avoid the multiple DLookUps with eachTreeView.Nodes.Add call for every row of recordset! At the very least store your DLookUp value once and apply it to treeview. This cuts down the repetitive 4 calculations down to 2 for every row. But since DLookUp essentially is a query anyway, simply integrate a JOIN to Master object in SQL queries of OpenRecordset calls....


3

Regarding your questions: Is an early return better for the first method? Or is it just a question of personal taste? The alternative you suggest would be: private void deleteMessage(CommandSender sender, int messageId) { int deleted = DatabaseMethods.deleteMessage(messageId); if (deleted == 0) { sender.sendMessage(MessageManager....


3

One of the requirements is that the coordinates of the player are stored, when the message is sent, and that it is stored in a sql table, so that admins on multiple spigot servers can access them independently of where the message was sent originally. Which raises questions about transactions and concurrent access. CREATE TABLE IF NOT EXISTS Meldung ( ...


3

You can greatly simplify things as well as wildly improve performance by using something like the following... DECLARE @start_date date = '2020-09-24', @interval_in_days int = 30, @appt_count int = 100; WITH cte_n1 (n) AS (SELECT 1 FROM (VALUES (1),(1),(1),(1),(1),(1),(1),(1),(1),(1)) n (n)), -- 10 cte_n2 (n) AS (SELECT 1 FROM cte_n1 ...


3

Your querying practices look secure to me. Your are using a prepared statement with bound parameters and using password_hash() and password_verify(). It really can be as simple as doing those basic/essential things. As for other refinements, I recommend writing the failure branches before successful ones, doing early exit()s, and only passing back an empty ...


3

Externalize your DML Given the length of the query written in GetArticleChangesAsync, I would expect that either it be moved to a stored procedure (common but IMO overkill); or moved to a view (my usual preference given read-only queries like this). There are several reasons for this, including: If you have a DBA, they will have more control over the fine-...


3

Overview OOP vs Procedural mysqli Firstly, it's important to note that the difference between the two methods is the way in which you, as a developer, interact with them. They both use use OOP one just hides it behind a veil of procedural functions. In fact, you can mix and match: // OOP $query = $mysqli->query("SELECT * FROM table"); while( $...


3

Initial Feedback The code looks quite modern - with return types declared, chaining supported, etc. The code is somewhat in-line with PSR-12 and thus is quite readable, though a few rules aren't followed - e.g. 4.4 Methods and Functions Method and function names MUST NOT be declared with space after the method name. The opening brace MUST go on its own ...


3

If you can with MySQL 8.0, use the WITH Common Table Expression to simplify this query. https://dev.mysql.com/doc/refman/8.0/en/with.html The idea is to extract out the nested queries into ones in the WITH statement before the main query and then use them just like tables in the main query.


3

If you look at the visual table diagram on the site you linked, for the Company table, the id is the primary key and therefore is guaranteed to be unique for each row in the table. name, on the other hand, is not guaranteed to be unique. So in your query, you should be grouping and deleting companies by their id, not by their name. Your query seems to be ...


3

Is the application well-structured or should I move the logic into controllers? I agree with NemoXP that the current format is not well-structured. A file with 112 lines of code isn't horrible in terms of file length, but it handles multiple things - e.g. routing, updating models, etc. Separating the code out to controller methods would be wise - especially ...


3

Welcome in the fantastic world of development. There is this great acronym in software development: S.O.L.I.D.. You can refer to it at any time to evaluate the quality of your code. However, in a two classes project, many of them are overkill. But there is one that you should try to use at all stages: "S" for "Single responsibility" ...


2

The query title is Questions answered by more than 3 50k users But that doesn't describe what the query actually does. The query selects questions with three answers given by three distinct top n rated users. They happen to have more than 50k rep on that site, but that's a mere coincidence. That can also be done by this query, containing only 1 subquery to ...


2

Before anything, read on SQL parameterization and avoid concatenating or interpolating variables directly in SQL statements. Otherwise you open your application to SQL injection and inefficiencies. The module, psycopg2, has much support for parameterizations as their docs indicate. Secondly, save cur as another self. attribute and handle cursor and ...


2

See @svick excellent answer, but, note that it does not account for regex special syntax character escaping. Literals like . are special selectors in regex. So, for a LIKE fragment asdf.qwer% a (python syntax) regex re.match('^asdf.qwer.*$', 'asdfXqwer') would INCORRECTLY match because . means "any single character here". You also need to escape ...


2

Welcome to Code Review, I have seen your code and I have seen you encapsulated your createQuery method like below: try { body method } catch (Exception exception) { exception.printStackTrace(); } You are basically catching all NullPointerException cases and other runtime exceptions you can meet within your method with the Exception class, this is a sign ...


2

Good stuff lots of good comments indenting your function definition putting variables inside of double quotes when you use them Suggestions indent between then and fi, as you would in other languages with conditionals combine then with if line using ; use [[ forms of conditionals to avoid surprises Those three things would lead to code like: if [[ ! -d &...


2

The PDO::quote() function is not correct for making column names safe. It's basically a wrapper for the QUOTE() function in MySQL: The string is returned enclosed by single quotation marks and with each instance of backslash (), single quote ('), ASCII NUL, and Control+Z preceded by a backslash. If the argument is NULL, the return value is the word “NULL” ...


2

Private Function connectionSuccessful() As Boolean The name suggest that you are testing to see if the Connection has already been opened when in fact it is used to opening the Connection and test if it was successful. Private Function OpenConnection() As Boolean This name tells you that you are opening a Connection. Since the return type is Boolean, ...


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