Hot answers tagged

14

Note: I don't think this is what you want to hear. But I have to write "the truth". This is my personal opinions about your code. Give me parameterized SQL or give me death! tell me if there are any bugs or improvements to be made. Don't suggest prepared statements. I'm sorry, but those two requests really don't fit together in the same Code ...


14

Well, I don't know much about PHP; but I know a little about security so that is what I am going to review. First I am going to review your encryption algorithm: AES-192. In June 2003, the U.S. Government announced that AES could be used to protect classified information. So that means you are doing okay/decent with your choice. High speed and low RAM ...


11

This part is duplicated: $orderBy = array_map(function() { return mt_rand(); }, range(1, count($id))); array_multisort($orderBy, $id); You apply the same logic to $id, $status, $online. To eliminate duplication, you can move this common logic to a function with one parameter: function reorder(&$arr) { $orderBy = array_map(function() { ...


10

You've made a good start, but the switch statement in userLanguage() just doesn't feel right from an Object orientated perspective: A class called Language should represent a single language which is generic enough to fit all cases: however at the moment, whatever language it represents, it has the definitions of all of them just jammed into that one method....


10

Extract some constants: replace // 70% chance of red an 30% of green $status = array( array("red", "no"), array("red", "no"), array("red", "no"), array("red", "no"), array("red", "no"), array("red", "no"), array("red", "no"), array("green", "yes"), array("green", "yes"), array("green", "yes") ); by: // 70% chance ...


10

Give me parameterized SQL or give me death! Seriously. Use prepared statements! Don't use mysqli_query as that makes your code vulnerable to SQL Injection. By using prepared statements, you won't need the mysqli_real_escape_string calls anymore. This code looks nasty: $result1 = mysqli_query($con, $sql); if (!$result1 || mysqli_query($con, $sql)) { die('...


9

Yes, the original code is awful. It is 200 lines of spaghetti code; I have no idea what it does, it is impossible to reuse and extremely difficult to test, debug, or change. That doesn't necessarily mean that the code should be changed, especially not without discussion, but that's a bit off-topic here. Regarding your code: Return early By adding guard ...


8

Because it's a framework static methods are used as my preferred method to access a class from multiple classes without having to worry about creating variables of instances that I either have to remember globally or re-instantiating the class constantly. I don't do php, but I'll give this one a try. Being a framework shouldn't be a reason to prefer static ...


8

Isset should not affect performance. About code improvements I suggest you to write some kind of wrapper above Request to improve readability <?php class RequestData { public function __construct($val) { $this->val = $val; } public function raw() { return $this->val; } public function safe() { // just an ...


8

You need to decide on the Domain Objects that represent concepts in your current model. A Route object should encapsulate the data to do with a route. If you look at Symfony's Route you'll see that it is simply an object with some properties, getters and setters, and that's about it (apart from a few select helper functions). This is also known as an Entity,...


7

I have done translation of one of my sites into English, French and German and what I found is that maintaining a class with getter's and setter's for a handful of phrases works, but anything more, just becomes hard work I like @MrLore parse_ini_file() approach. with a few differences For example instead of using made up constants like PAGE_TITLE, use ...


7

If the second or later query fails you'll have inconsistent data in your database. Consider using atomic transactions. $query_2, $query_3 are bad names. You could pick something more descriptive, like $usersInsert, $freelancersInsert etc. $query = $this->db->prepare("INSERT INTO " . DB_NAME . ".users (firstname, lastname, email, email_code, ...


7

I have been wanting to answer this for a while now, and I think I can give an answer today! So... Am I using PHP Mcrypt the right way? Well, what is right? Hows about, what is wrong? There's a blog I know of, which is written by a pretty well respected man on StackOverflow, and he published a post back in late 2012. It covers the basics of the most ...


7

I'm assuming you want your script to output a list of user ids, usernames, and online status. Each user has a 30% chance of being online and the list changes randomly every 5 minutes. If I'm wrong about any of that please correct me. These lines of code don't affect the output. $start = mktime(0, 0, 0, 3, 1, 2014); $rate = 1 / 30; $amount = number_format(...


7

Let's look at all of your questions, one by one: This class doesn't really do anything, and is really a pretty struct Yes and no. This class has a clear task within the application: it is passed around, carrying data. It represents a number of values that belong together. It's a model. Having data containers isn't just OK, it's good practice. You can use ...


7

Two suggestions on how to keep this code maintainable: use early exits keep conditions atomic (one if checks for one thing) Example: function findLoggedInUser() { if (($userData = $this->httpSession->getParameter('user')) && isset($userData['id'])) { // User found in current http session return $this->userMapper->...


7

Use Guard Expressions The proper way to do this is to use Guard expressions. If you have a number of conditions that need to be true before you take an action. You don't check them all in a nested manner but instead check their negations one and one and return/break if you find any condition that is not met. Example Instead of having: if(a){ if(b){ if(...


7

The code is horrible. It probably contains all of the top 10 vulnerabilities listed on the OWASP web site. It uses a programming language that is well-known for its bad security history and its awful API that makes it difficult to write secure, solid code. Forget about that tutorial, warn others about it, find a better tutorial with focus on good code and ...


6

Your code isn't exactly terrible, but this is code-review, and good code review has to be harsh, so please keep in mind that I mean this in the nicest of ways. That said, there are some issues I'd like to address: Redundancies - DNRY: That old dead horse: Do Not Repeat Yourself. You have a lot of redundant code. Of course, you can get into code-golfing, and ...


6

I see several problems here: It's hard to understand your code because you have not documented any of the methods and their arguments. I have trouble understanding what's the purpose of $event, $key and $callback arguments. First off I though that $callback must be a the callback function, but from code it looks like it's an array of some sort. What should ...


6

I have rewritten the script in another fashion. Beware all your code is almost exactly the same, but not entirely. If you are looking to just improve your script a bit, look closely at the parser. A number of personal preferences I would like to share that I thought while reading your code but which I did not fix in my own code in all cases: I like (foo ===...


6

I don't think its really SQL-injection, but $hostname is not taken from server but from DNS. Theoretically i can add some kind of malicious domain name and affect your query. But from other hand, domain syntax is really limited. I don't think you can use it to do some serious thing. Though i would recommend some error protection to avoid SQL-errors. And ...


6

To start with, your code doesn't make much sense to me. You're looping over $this->service_results->results[0]->address_components, and on each iteration, you seem to be setting all of the address values. If the address_components array contains 4 arrays, each setting all of the address vars, you're effectively overwriting the values, and are only ...


6

I'm not very familiar with PHP so this will only cover some basic observations. Readability. On the plus side, your code is full of comments explaining what everything does. On the minus side. It also suggests you're not separating things out. Really good code should never need to explain what it is doing. It should only ever need to explain why something ...


6

Maybe you want something short like is echo ($n <= 1) ? 0 : pow(10, $n - 1);


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


6

I am bit puzzled by strip_tags: $em = strip_tags($_POST['reg_email']); // remove html tags $em = str_replace(' ', '', $em); //remove spaces $em = ucfirst(strtolower($em)); //uppercase first letter I have the impression that you stumbled upon incredibly old tutorials. You don't use that function to check user names or E-mails, because you are not expecting ...


6

Please read about sql injection. Basically you should be using prepared statements with parameters for executing SQL queries - either PDO::prepare() or mysqli_prepare(). If that isn't enough of a clue, consider the case where $_POST['body'] contains something like body');DROP TABLE users; or some variant that leads to successful WHERE conditions that end up ...


5

Starting from the top… Common idioms for repeating a loop 20 times are: for ($i = 0; $i < 20; $i++) { # Do something } or for ($i = 1; $i <= 20; $i++) { # Do something } What you wrote, with 21, is something I'd consider weird and jarring. (The same comment applies to your SQL query — you should write <= 20 instead.) allFieldsFilled() ...


5

This Function can be shorter. public function qdelete(){ $tbl = self::prepArgs(4, func_get_args()); $this->db->delete('Storage_Users'); if($this->db->affected_rows() > 0){ return TRUE; }else{ $this->msg = 'There was an issue removing that record.<br />' . $this->db->_error_message(); ...


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