-2

I'm referring to using the die() function for something else than debugging. This is a "well it works" situation, but is it bad practice?

use Symfony\Component\Filesystem\Filesystem;
use Symfony\Component\Filesystem\Exception\IOExceptionInterface;

/**
 * This Command will help Cjw create a new demo site to start off
 * with the Multisite bundle.
 *
 * Class CreateSiteCommand
 * @package Cjw\GeneratorBundle\Command
 */
class RemoveSiteCommand extends ContainerAwareCommand
{
    private $vendor; //eg "Cjw"
    private $fullBundleName; //eg "SiteCjwNetworkBundle"
    private $fullBundleNameWithVendor; //eg "CjwSiteCjwNetworkBundle"
    (more vars)

    /**
     * this function is used to configure the Symfony2 console commands
     */
    protected function configure()
    {
        $this
            ->setName('cjw:delete-site')
            ->setDescription('Delete Cjw Site')
            ->addOption('db_user', null, InputOption::VALUE_REQUIRED, 'If set, the database user will be shown on the instructions');
    }

    /**
     * This function executes the code after the command input has been validated.
     *
     * @param InputInterface $input gets the user input
     * @param OutputInterface $output shows a message on the command line
     * @return int|null|void
     */
    protected function execute(InputInterface $input, OutputInterface $output)
    {
        // set optional db_username

        $dialog = $this->getHelper('dialog');
        $reusable = new \Reusable();
        $fs = new Filesystem();
        $cmd = new \ColorOutput();

        //push only vendors into the vendors array
        $vendors = $reusable->getFileList($reusable->getMainDirectory('src'),true);


        //select a vendor from the array
        $vendor = $dialog->select(
            $output,
            'Select your vendor [1]: ',
            $vendors,
            1
        );

        $bundles_in_vendor = $reusable->getFileList($reusable->getMainDirectory('src/'.$vendors[$vendor]),true);

        //push bundles that start with 'Site' into array
        $sites = array();

        foreach($bundles_in_vendor as $bundle)
        {
            if($reusable->startsWith($bundle,'Site'))
            {
                array_push($sites,$bundle);
            }
        }

        $site_to_delete = $dialog->select(
            $output,
            'Select site to remove: ',
            $sites,
            1
        );

        $bundle_deletion_path = $reusable->getMainDirectory('src/'.$vendors[$vendor]).'/'.$sites[$site_to_delete];

        $are_you_sure = array('yes','no');
        $confirmation = $dialog->select(
            $output,
            'Are you sure you want to delete: '.$sites[$site_to_delete],
            $are_you_sure,
            1
        );

        if($are_you_sure[$confirmation] == 'yes')
        {
            echo $cmd->color('yellow','attempting to remove bundle in: '.$bundle_deletion_path);
            $fs->remove($bundle_deletion_path);

            //returns demo
            $sitename = strtolower($sites[$site_to_delete]);
            $sitename = substr($sitename,0,-6);
            $sitename = substr($sitename,4);
            $this->setRawSiteNameInput($sitename);

            // returns acmedemo
            $symlinkname =  strtolower($vendors[$vendor].substr($sites[$site_to_delete],0,-6));
            $this->removeSymlinks($symlinkname,$this->getRawSiteNameInput());

            $this->createSetters($vendor,substr($sites[$site_to_delete],0,-6));

            $this->deleteEzPublishExtension();
            echo $this->getFullLegacyPath();

            echo $cmd->color('green','deletion process completed.');
        }
        else
        {
            echo "deletion canceled";
            die();
        }

        function_that_further_deletion_process();
    }

This is a symfony2 console script that removes a site from a certain structure

user3531149
  • 1,519
  • 3
  • 30
  • 48
  • 2
    You have no function in your code. I don't think it works, or when the variable is undefined you want to write any text into a blank page? – pavel Feb 10 '15 at 10:36
  • @panther I posted the whole code now, I used the other one for the sake of simplicity – user3531149 Feb 10 '15 at 10:40
  • Specifically for Symfony, it's not the best practice, as this might prevent some event listeners to be triggered further down the line. – Gerry Feb 10 '15 at 11:58

2 Answers2

2

That is perfectly safe, if that is your question, since php as a quasi interpreted language does not leave any traces or artifacts when terminating the execution.

If it is a good practice is another thing. I'd say it is fine for testing purposes, but you should avoid it in final code. Reason is that it makes code hard to maintain. Consider someone else diving into your code and trying to understand the logic. One would actually have to go through all the code to stumble over this detail. Chances is one does not, so the behavior of your code appears broken.

Instead try to do one of these:

  • throw an exception to leave the current scope. Such an exception might well be caught and swallowed by the calling scope, but it is a clear and predictable way of returning. Obviously you should document such behavior.

  • return a value clearly out of scope to what would "normally" be returned. So for example null or false instead of a typical value. This forces the calling scope to check the return value, but that is good practice anyway.

  • restructure your code such that there is no reason to suddenly terminate the execution.

arkascha
  • 41,620
  • 7
  • 58
  • 90
  • I think you should mention a method to gracefully kill the application run. Either by setting a custom error/exception handler, or some other technique? – ʰᵈˑ Feb 10 '15 at 10:39
  • the thing is, exceptions are usually used when errors are found, not when the users cancels an operation on demand.. – user3531149 Feb 10 '15 at 10:42
  • @user3531149 I disagree, that is a question of the viewpoint. An exception is not equal to an error. It only means: something unexpected has occurred. Not more, not less. I often use exception to signal things up the stack. It makes the code readable and crunchy. Obviously you want to use a custom exception class for this. – arkascha Feb 10 '15 at 10:44
0

You haven't told us what your intention was with that die.. consequently we cannot tell whether you're using the right tool...

die and it's synonym, exit both exit the script. If that's what you want, it's fine.

I tend to use exit for normal operations, and die for erroneous situations (something you cannot properly recover from, e.g.: couldn't connect to database). I also use a wrapper for die so if needed, I can log such events. Though both commands do the same thing, there's a difference in intention, and I want to express that intention in the code.

In your example I would use exit.

Karoly Horvath
  • 94,607
  • 11
  • 117
  • 176
  • That's code, not intenion. Code does what it does, and not necessarily what you've imagined it will do... But from the context I assume you're doing the right thing... print error message, exit. – Karoly Horvath Feb 10 '15 at 10:42