Hot answers tagged

68

Personally, I must say that close to all PDO derived/based classes I've seen so far suffer from the same problem: They are, essentially, completely pointless. Let me be clear: PDO offers a clear, easy to maintain and neat interface on its own, wrapping it in a custom class to better suit the needs of a particular project is essentially taking a decent OO ...


19

Your query is simultaneously awful and awesome. It is truly impressive, especially if you are self-taught. Unfortunately, I don't believe it is correct. (Or, I may have misunderstood how you intended for the vote redistribution scheme to work.) For example, it reports that the redistribution in favour of Candidate 2 should be +0.125. Of the 8 ballots ...


18

Looking at your code, there are several things that immediately I can tell are issues. Before we even get to performance, there are basic security measures that need to be taken first. SQL Injection Like you've seen in the comments, your code is highly susceptible to SQL injection. But, what is it exactly? By definition, it is the insertion or "...


17

It feels like you are combining lots of hashing and string transformation in hopes that complexity will make your scheme more secure. That's not engineering or computer science, though — it's Cargo Cult Programming. There are two ways that an API key scheme could work: To generate the API key, concatenate the username with a salt and site-wide secret, ...


17

On demand -> issues mentioned in comments, reviewed & posted as an answer: The answer to your question has already been given: Yes, you have a serious vulnerability, and the first step to solving this is using the extensions that aren't deprecated, like PDO and mysqli. Both of these extensions support the easiest, and pretty solid (not perfect, but ...


16

Use prepared statement You should never ever formulate an SQL string from variables. You should use parameterized prepared statements, and let a library fill in the parameters using the right native types. Instead of this: string query = "SELECT CNIC FROM `voters`.`voters` WHERE ID=" + k.ToString() + ";"; MySqlCommand cmd = new MySqlCommand(query, def)...


15

More important than the performance, your code is very likely vulnerable to SQL Injection. (I can't say for sure without looking at your PHP code) If $viewingUserId or $userId is chosen by the user in any way, then you have a problem. And even if it is not, I would highly recommend using prepared queries. I don't know if you're using the mysql_* methods or ...


14

I think this is better than about 99% of first tries I've ever seen! Anyway, I've tried my best to address as many things as I saw, whether major or extremely minor. If you have any questions on any points or want to clarify or defend any claims, let me know and I will expand the answer. Assuming array indexes exist Any array that comes from user input ...


13

Preamble As a matter of security, I can safely say that you would fail a professional security audit in seconds. You should be including the hashing library as an external component. You also need to forget everything you know about querying databases and start again. Also, look at Model-View-Controller. The fact you have a templating engine and separate ...


13

Overall it's not bad, but here are a few things that stuck out for me. I'd consider renaming getBalance to getBalances, since you're actually returning the balances from multiple bank accounts for that user. balance = bankAccount.getBalance(userId); Consider creating and returning an array of AccountBalance type from the getBalance method instead of an ...


13

In addition to all the other good points mentioned: This code massively mixes up presentation logic, business logic, and back end database implementation details. Those are three separate things; they should be kept far apart in the code. This code has "sql injection attack" written all over it. Don't ever use strings to build up SQL queries. Use stored ...


13

Here are some observations: you first iterate all rows in Credentials and then select the Password yet again - why not just select the pasword? seems like your password is plain-text - BAD idea - hash your password together with a salt please don't concat strings to get SELECT queries - use parameters instead you are using C#/ASP.NET so why don't you use ...


13

What my main question is, is since I'm new to JPA if I'm using an efficient way to insert the data fetched from the Twitter API in my database. Well, it's not entirely bad, I understand that you need it because only your sqlmanagement.XXX classes are annotated with @Entity. You could improve the methods that you are using for it though. First a nitpick: ...


12

I'm not sure why you're using your own built classes to access the database when .net has a very complete set of classes that can be used for data access. Just looking at your syntax, I see the same functionality, so to me that is a waste of development time. Also, it seems while I don't have access to your classes locally, I've changed them to use the ...


12

Aliases Table aliases are handy, sure. But single-letter aliases are not good. It's OK to want to save having to type more characters than needed, but you have to keep in mind that things like aliases and variables get really confusing if the name you give it does not say anything about what it means. c, o, p, ot... Why not instead cust, ord, prod, ordItems?...


12

One of the nested loops can be eliminated, by joining these two queries: $query = " SELECT * FROM imagemap WHERE exercise = '".$data['ExerciseID']."' LIMIT 1 "; $result = $wpdb->get_results($query); foreach($result as $row) { echo '<div class="imageGridImages imageGridImagesLarge">'; global $wpdb; $exerciseTitleQuery = " SELECT ...


12

It is a maxim in computer science that there are really "only three numbers": zero, one, and many. You have a case of many, and you should treat it as such. Otherwise, someday a customer might want to buy a hundred of some food item, and performance will be poor. Therefore, I recommend… Option four Instead of having the table represent current supply ...


12

Style Python has a style guide. It's called PEP8, and normally if you follow it you get easier to read code. It's a fairly small read, and you can learn some new things from it. It's recommended in PEP8 to limit the character width of scripts to 79 characters. It has a few benefits: People on StackExchange don't need to use the horizontal scroll. It's ...


12

There is multiple things that can be optimized in this code. It is pretty obvious that most time is spent in HTTP requests to remote API and in database queries, so these points should be first in list of optimizations. Your code doesn't tells much about what is coming from remote API and you didn't tell if you really need to run all requests for this API ...


12

You can create a file db.js var mysql = require('mysql'); var settings = require('./settings.json'); var db; function connectDatabase() { if (!db) { db = mysql.createConnection(settings); db.connect(function(err){ if(!err) { console.log('Database is connected!'); } else { ...


11

What you've got is pretty readable, but there are a few points to make: Your sub-query is written with a different indentation style from the main query. I prefer to see the keywords in upper-case. Personally, I dislike spaces before commas intensely (and you aren't 100% consistent about adding them). However, I loathe even more the style where the comma ...


11

Array based approach Instead of appending a number to the field, use an array based approach: <form action="" method="POST"> <b>Product 1</b><br <label>Product Id <input type="text" name="products[0][id]"></label> <br> <label>Product Type <input type="text" name="products[0][type]">&...


11

Agree entirely with Corbin's post, these are just additions. A point on preference rather than efficiency, but I find it odd to have plain, unaltered HTML echoed in PHP. I personally would escape it, but again, preference. At least you are using heredoc :) I'm assuming $_POST['clicked'] is the same concept as checking for a submit button press. This is not ...


11

Congratulations, that means you are learning. You'll be at that stage for some time. In fact, I don't think you'll ever truly leave it, or if you do, then you should be concerned. The time between "changes" might increase, but you should never stop learning and should always find some way you can improve. Its only natural. The first few months I was learning ...


11

I think there are two major issues you should address right in the beginning when you start with PHP. Separation of Layout and Logic This means in general that you shouldn't mix HTML and PHP code. Later this will lead you to the MVC-Pattern. index.php <?php // connect to database include("inc/dbconnect.php"); if ($_SERVER["REQUEST_METHOD"] == "POST") ...


11

Disclaimer: I'm a SQL Server guy. I might get the particular syntax of MySQL wrong. I apologize if I do. The first thing I notice is this that subquery shows up over and over again in your query. SELECT FLOOR((COUNT(*) / (2 + 1)) + 1) threshold FROM votes You should use a variable to store the result of this at the beginning of your query and call it each ...


11

Let us review! Why are you strip_slashing and strip_tagging the username and passwords before they enter the database? Why do you care? (You don't, I'll get to that in the end). Calling __construct() inside of another method of the same object is not recommended. (What does __construct() do? As opposed to what do setUser() and setPassword() do) Hashing your ...


11

Ok, so the main problem with your code is that it is vulnerable to SQL injection you can fix that by using prepared statements - http://php.net/manual/en/mysqli.quickstart.prepared-statements.php If you want to remove the ?l= you can look into Mod Rewrite - http://www.sitepoint.com/guide-url-rewriting/ and have the URL generate as gus.netii.net/$hash I ...


11

This is going to be a bit of a drive-by code review due to time limitations and my limited php knowledge, but hopefully I can add some value here. Here are some high-level things I've noticed about this code: PARAMETERIZE YOUR QUERIES!. This is the most common security hole found on the web, so make sure you understand the problem and how to combat it. Use ...


11

Is my code properly commented? No. You're documenting methods where they are used (usually multiple places) as opposed to where they are declared (inherently only one place): /** * @param associative array * stripAllWhiteSpaces will remove ALL white spaces. * example: $stringBefore = ' this is an example'; * $stringAfter = 'thisisanexample'; */ $...


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