Make code more memory efficient, or shorter

Question

//rename files    
    if(!empty($_FILES[$desktop_fieldname_1280x800]['name'])){
                file_exists($desktop_filename_1280x800 = mb_strtolower($desktop_dir_1280x800 . 'wall_desktop_1280x800_' .
                $_FILES[$desktop_fieldname_1280x800]['name']));
            }   
    if(!empty($_FILES[$desktop_fieldname_1366x768]['name'])){
                file_exists($desktop_filename_1366x768 = mb_strtolower($desktop_dir_1366x768 . 'wall_desktop_1366x768_' .
                $_FILES[$desktop_fieldname_1366x768]['name']));
            }   
    if(!empty($_FILES[$desktop_fieldname_1920x1080]['name'])){  
                file_exists($desktop_filename_1920x1080 = mb_strtolower($desktop_dir_1920x1080 . 'wall_desktop_1920x1080_' .
                $_FILES[$desktop_fieldname_1920x1080]['name']));    
            }

With the above code, basically what I'm doing is renaming the files properly + consistently. What my code seems to be bit redundant to me, I think this can be done with some kind of loop or arrays, but I'm a newbie and not sure how it will be. So would like to ask your help to make this code more memory efficient, or shorten the code in some way.

Answer 1

The code does not rename any files, it modifies the values of certain variables.

Using assignments as a parameter to your functions is confusing.

You call file_exists but do nothing with it. It is supposed to be used to check for the existence of a file. Something like:

if (!file_exists($destination))
{
    rename($source, $destination);
}

To answer more you would need to show how the desktop_dir_xxx variables are calculated.

Answer 2

Firstly you're certainly not doing any file-renames in that code. As Paul mentions you call file_exists but that function only computes a value (which you ignore). It has no side-effects (other than the assignment in the parameter to file_exists, which as Paul also mentions, is bad design), and hence your code is just an elaborate variable assignment, like this:

    if(!empty($_FILES[$desktop_fieldname_1280x800]['name'])){
            $desktop_filename_1280x800 = mb_strtolower($desktop_dir_1280x800 . 'wall_desktop_1280x800_' .
            $_FILES[$desktop_fieldname_1280x800]['name']);
        }

Secondly, your code can be made significantly more readable with a loop, or perhaps a function. Your code all seems to be of the form:

    if(!empty($_FILES[SOMEVARIABLE]['name'])){
            SOMEVARIABLE = mb_strtolower($desktop_dir_1280x800 . 'wall_desktop_1280x800_' .
            $_FILES[SOMEVARIABLE]['name']);
        }

And you can of course factor that out into a function where SOMEVARIABLE is a parameter to the method.

Finally, you're talking about making your code more memory/CPU efficient. It is always always always better to think about making your program more efficient to read than making it more efficient to run.

The reason for this is slightly non-intuitive for new programmers, but basically it comes down to the fact that programming is an easy part of a programmer's job. It is much harder to debug and maintain sloppily written code - and any steps that you take now to make your code more readable - even at the expense of actual efficiency - will pay of many-fold later - and it's a good habit to get into early in your programming career.

For this reason, unless there is a specific pressing concern about efficiency (i.e. your program is running unacceptably slow), spend your spare time making your code more readable, and leave CPU and memory efficient optimizations for later - and then only after you've specifically benchmarked to discover the performance problem in your design.

Answer 3

I don't really understand how you got to that code... Doesn't make too much sense to me.

As a principle, you can put that in a loop in the following way:

$sizes = array('1280x800','1366x768','1920x1080');
foreach($sizes as $size) {
    $var = 'desktop_fieldname_'.$size;
    if(!empty($_FILES[$$var]['name'])){
        //[...]
    }
}