Hot answers tagged

26

People who want to teach other should not write code like this. Not even slightest indication that you understand what Separation of Concerns is. The pattern that you are using is called Big Ball of Mud. You have everything in the same file. No understanding of HTML. The code you wrote is not valid, it uses tables for layout and completely ignores semantics....


18

Are you a horrible programmer...no probably not. Is there room for improvement? There usually is. here's my two cents: I see one factory class, but then you have a whole-lotta creation work going on throughout the rest of your code new fMoney, new TaxBracketManager, new TaxBracket, etc... When you hard code these dependencies into your methods how ...


17

If you are worried about modifying the array while looping, don't because foreach copies the array. Try $array = array('foo'); foreach ($array as $k => &$item) { $array[] = 'bar'; } var_dump($array); And see it terminates just fine. foreach ($array as $k => &$v) is a shorthand for foreach (array_keys($array) as $k) $v = &$array[$k] so ...


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


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


13

Veilig is right and made some very good points +1. There's nothing "horrible" about this code. Everyone can stand to use some improvement somewhere, even professionals. So saying this is "horrible" is just naive and immature. Don't listen to anyone with such negative criticism, especially if they aren't willing to explain themselves. Even constructive ...


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

This sounds like a place for array_map(). $array = array_map('perform_changes_on', $array);


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


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

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


6

This code seems to be very good as far as performance is concerned. I cannot see any way to improve it. I used xdebug and cachegrind to see that my test samples were bound by the performance of the loop that creates the tokens. I investigated switching the $i and $j loops around, but the performance was no better. I cannot see any way to improve the code,...


6

Note: I'm sorry if this is completely irrelevant but I'm not entirely sure what you are trying to accomplish. If I'm way off base please let me know. When I create a template loader I generally use output buffering with php include. This allows you to run the page as a php file without displaying its content before you are ready. The advantage to "parsing" ...


6

First, a few minor comments: Class properties should almost always be protected or private. This forces you to use a known tested interface (the class methods) to interact with the state stored in the class. The properties section, something and validation should have all been protected or private. Your method add does not add, it sets. add might be ...


6

I'm not sure what you want to achieve with the constructor. public function __construct($var1,$var2, $var3) { $this->_var1= $this->$var1; $this->_var2= $this->var2; $this->_var3= 8; // i need this 8 if no value is passed } If you want to set the properties to the given parameters, you should do $this->...


6

I'll probably revisit this later, but for now: The exception catching should be done at either the model or model-using layer. I would probably let the exceptions bubble up through the model unless you want the model to be fairly high level. For the sake of abstracting the DB layer away from the model layer, I might consider wrapping the PDOException in ...


6

Instead of accessing the employee key every time, just pass in the array rooted at the Employee key What happens if all of the array indexes are not set? You should either check for that or use an object on which you know they all exist Important mostly for data integrity (what if you create an Employee array somewhere and forget a status or something?) ...


6

Immediately I see globals. Avoid globals at all cost. Strike them from your memory. You will never need them. Ever. Globals are one of the worst things PHP has ever done. It is not always obvious when a variable is a global thus it is very easy to miss and sometimes impossible to trace. It is also really easy to change the value of a global from anywhere, ...


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


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