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


6

Okey, first of all; I had a hard time determining if this was a login script or a registration script even if you explicitly said it was a script containing login validation. If I was you I would review the code and try to clear up any vagueness, because I got a headache trying to sort your code out. Here we go anyway: At line 6: Why do you use xss_clean() ...


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(); ...


4

This a nice example: it shows how the MVC is just a pattern which should be ajusted to your needs. The menu items you're showing seem very unlikely to change over time. This means you can simply put them in your "templates/menu" view. If they're shared among multiple views, then the controller is OK. If the menu items can change dynamically, then your model ...


4

I've already reviewed some of your other code, so I will only add new items in an attempt not to repeat posts Booleans There is no reason to recreate a feature that already exists. By checking for a "yes" or "no" or a 1 or 0 value you are recreating a boolean. Simpler to just use TRUE/FALSE this way you can check your variables like so. if ($this->...


4

I know you are specifically asking about just the code between the commented section, but I feel the rest deserves some attention as well. I will try to focus primarily upon what you requested though. Separating Your Code First off, there is no reason for all that code to be contained in a single method. Separate your code into smaller pieces so that they ...


4

Well, for a kickoff, you're concerned about your code's reusability can trump its undeniable downsides. But, if you look at one or two things more closely, your code isn't as reusable as perhaps you think it is: // Parsing arguments $params = (isset($args[0])) ? $args[0] : array(); $single = (isset($args[1])) ? $args[1] : false; $count = (isset($args[2])) ? ...


4

Formatting is not consistent: $this -> load -> model('user_login_model', 'login'); $user_id = $this->session->userdata('user_id'); Sometimes there are spaces around ->, sometimes aren't. You could use a guard clause to make the code flatten: $user_id = $this->session->userdata('user_id'); if ($user_id != TRUE) { return; } if (...


4

As Pimgd stated, all your request are synchronous (and wait that the previous one terminates before executing further). To make the calls asynchronous I see two possibilities: either the client (browser) does it: you send an almost empty page (with the token) and some Javascript code to perform the API calls (asynchronously). or the server does it and I ...


4

I had a very short look at lightblog/application/controllers/Pages.php and I noticed that these two values were not used in the page() method: $data['pages'] $data['posts'] Then I realized that you hand over data to other objects and they might be using these two values. Who knows? This makes your code very hard to understand and to debug. Basically you'...


4

There are a number of things I'd do differently and the first is a must. First, using md5() to encode a password is universally considered a bad practice. It's been considered too weak for that purpose since at least 2004 if not earlier. Instead, use the PHP function password_hash. I would encrypt the password in the model. In fact, I'd move almost all ...


3

First of all you should use site_url() in your jquery code instead of base_url() as it provides url flexibility. For more info visit http://codeigniter.com/user_guide/helpers/url_helper.html And in your controller code i suggest you to use bulk insert instead of loop insert. So you may use this piece of code: foreach( $pieces as $user_id2) { $...


3

use this $this->db->where('`store_id` NOT IN (SELECT `store_id` FROM `user_store`)', NULL, FALSE); Check this


3

is this iteration pattern safe? Safe for what? Safe for a database? We can't tell you that! There's no code for your database entry function. What you have here is perfectly safe, but that's excluding any type of insert into a database. To safely protect yourself from SQL injection and other types of security exploits, use prepared queries, validate (...


3

How about the following: if (!$salt) { $salt = bin2hex(mcrypt_create_iv(32, MCRYPT_DEV_URANDOM)); } elseif (strlen($salt) !== 64) { log_message('info', 'Supplied password to process_password() was not the correct 64-byte length.'); return false; } // if we're here, we have a valid salt Improvements Removed $salt from second condition (already ...


3

Simply use return $this->simpleloginsecure->login($email, $password) in the login method. redirect('/user/xxx') is enough, since location is the default redirect type. Since you're doing a redirect(), $data['message']is probably "dead code" here. Use CodeIgniter's Flashdata instead. <p class="error"> should be handled in the view, the controller ...


3

Another recommendation apart from all of the good ones you have already received. Don't create variables that are not variables. // Assign variables to returned data $passwordDB = $user_data->password; $passwordDB2 = $user_data->password2; $first_name = $user_data->first_name; ...


3

Some notes: 1, elseif ($user_data->users_statuses_id == 4) Instead of magic numbers you should use named constants. Consider creating an $user_data->isUserBanned() function also. The same is true for 'flags' => 3 and ($lock_date !== "0000-00-00 00:00:00") (the latter should be $user_data->isLocked()). It helps readers a lot and ...


3

First of all, thanks for sharing your code, this is the best way to improve yourself! The M in MVC A Model in any MVC framework should provide an easy way to access a specific type of data. It allows you to stop worrying about database details and only access your specific type of data in a meaningful way. For a StackOverflow-like website, this means that ...


3

From a CodeIgniter point of view, this is fine but CI isn't really MVC. in MVC proper, the controller would be doing very little (See: https://stackoverflow.com/questions/13813046/how-is-mvc-supposed-to-work-in-codeigniter ). However in CodeIgniter's implementation you still want to strive for "Fat model, thin controller", which you're doing for the most ...


3

The access parameter in your doccomments is unnecessary. Any application that is sophisticated enough to provide doccomment hints is also sophisticated enough to tell you the access method automatically. At least I have yet to find one that contradicts this. Your function is doing entirely too much. The Single Responsibility Principle states that a function/...


3

I would like to add something to @Dukeatcoding comment in the question. What is the point of passing the whole object? Thinking on it as a part of the main controller, you should only pass the data as an array to the model: $data = array( 'id_user' => $usuario, 'id_category' => $soporte->getCategoria 'message' => $soporte-&...


3

Yes, you will miss every appointment that doesn't have :00 for seconds. And if your cron errs for any reason or skips a second, you'll miss even more. You should add another DATETIME column to your table called notified_at (or something like that) and then do two conditions in your where clause: WHERE start_datetime <= '$two_hour_time' AND notified_at IS ...


3

This might be classified as a micro-optimization but... As handy as the Query Builder is, it runs quite a lot of code putting together a query statement. So instead of $this->db->select(...)->from(...)->where(...)->get(); Write out the query in a more direct manner. //First get incoming result and search in the database $sql = "SELECT ...


3

Validating emails is hard, and it's not a good idea to do it on your own with a regex. Here are a couple of valid email addresses you would not allow (there are a lot more): "foo"@bar.com, foo+spam@bar.com, !#$%&'*+-/=?^_{|}~@foo.com. And even though it's so strict, it still lets through invalid addresses such as foo@example.com] (because the regex is ...


3

No, it doesn't work like that because you're not escaping your PHP strings to be a safe JavaScript string. What you're doing is encoding them as JSON but if - for example - $res->token contains \ then it will produce a broken JavaScript string and JSON.stringify() won't fix it. Correct code must use json_encode() for this purpose. echo '<script>...


3

What's happening here is that if each request takes 0.3 seconds (pretty fast), the whole page takes 14 * 0.3 = 4.2 seconds to load (slow-ish). In cases like this, there's two things you could do. Run each request asynchronously. Alter the API to give you everything. By running the requests asynchronously, you're not waiting for the response of one request ...


3

Looks pretty good. Here are just some thoughts but nothing major: It would be more future-proof (and probably efficient) to use regular expressions in lieu of parsing hard-coded line numbers (in case the data has rows added later): For the _save_data() you could parse like so: $string = '{THE CONTENT FROM THE COUNTRIES LIST PAGE}'; $sub = preg_replace('/.+?...


3

Let's start with your controller (controllers/Volunteers.php): You have loaded list of models in __construct() which is not a good practice. Before loading list of models in __construct(), you must be sure, if you need methods from all the loaded models in most of the methods of your controllers. You loaded all the models without giving them a short object ...


3

Looking at your sql's you seem to be repeating the same queries in a loop. I will pick out one simple one for example, it doesn't take long to run on its own, but if you add them all up, the amount of roundtrips to the db grows. 0.0006 SELECT COUNT(*) AS `numrows` FROM `volunteer_shifts` WHERE `shift_id` = '82' AND `shift_deleted` =0 If you can get ...


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