Conditionally truncating a filename to a max length without damaging the file extension Yes, I'm bad at conditionals. I don't use them so much and I feel a bit lost with them. How would you (if possible) refactor or change this little piece of code?

Second improved version:

$MAX_FNAME_LEN   = 255;
$clientFilename  = $this-&gt;file-&gt;getClientOriginalName();
$clientExtension = $this-&gt;file-&gt;guessExtension();

// Assume a failure in reading client filename
$defFilename = $defRandFilename = uniqid('fid_', true);

if(!is_null($clientFilename))
{
    $clientFilenameLength = strlen($clientFilename);
    $hasClientExtension   = !is_null($clientExtension);

    $removeFromEnd = $hasClientExtension ? - (1 + strlen($clientExtension)) : 0;
    $appendAtEnd   = $hasClientExtension ? ".$clientExtension" : '';

    if($clientFilenameLength &gt; $MAX_FNAME_LEN) // Truncate original filename
    {
        $excessFromEnd = $MAX_FNAME_LEN - $clientFilenameLength - $removeFromEnd;
        $defFilename   = substr($clientFilename, 0, $excess) . $appendAtEnd;
    }
    else
        $defFilename  = $clientFilename; // User original filename

    if(strlen($defRandFilename) + abs($removeFromEnd) &gt; $MAX_FNAME_LEN) // Truncate sanitized
        $defRandFilename = substr($defRandFilename, 0, $removeFromEnd) . $appendAtEnd;
    else
        $defRandFilename .= $appendAtEnd; // Use sanitized plus extension
}
Use early exit: reverse the null-checking conditional and return.

if(is_null($clientFilename))
    return;


If your code isn’t in a function, change that.

Next, use consistent indentation for the else: it belongs on the same level as the if, not as the content of the if.

I’d also encourage you to follow general usage in PHP, i.e. to use the braced if style even though I agree with you that the other style is fundamentally more readable.

I’d also get rid of unnecessary variables such as $clientFilenameLength, and of magic constants (255). And as a small clean-up, use string interpolation instead of concatenation where it makes sense.

That leaves us with:

define('MAX_LENGTH', 255);
$clientFilename  = $this-&gt;file-&gt;getClientOriginalName();
$clientExtension = $this-&gt;file-&gt;guessExtension();

// Assume a failure in reading client filename
$defFilename = $defRandFilename = uniqid('fid_', true);

if(is_null($clientFilename))
    return;

// Remove strlen($clientExtension) + 1 chars from the end
$hasClientExtension = !is_null($clientExtension);
$removeFromEnd = $hasClientExtension ? 1 + strlen($clientExtension) : 0;
$appendAtEnd = $hasClientExtension ? ".$clientExtension" : '';

if(strlen($clientFilename) &gt; MAX_LENGTH)
    $defFilename = substr($clientFilename, 0, -$removeFromEnd) . $appendAtEnd;
else
    // User original upload name
    $defFilename  = $clientFilename;

if(strlen($defRandFilename) + $removeFromEnd &gt; MAX_LENGTH) 
    $defRandFilename = substr($defRandFilename, 0, -$removeFromEnd) . $appendAtEnd;
else
    // Use random file name plus .ext
    $defRandFilename .= $appendAtEnd;


But this code probably doesn’t do what you want: the whole check for strlen($clientFilename) is a red herring because the result will not be truncated. It will remove (a string of the length of) the client extension, and subsequently replace it. Is that really what you intended?