2
\$\begingroup\$

I have the following function which handles requests. It has more than 130 lines

public function run() {
    $objectRequests = json_decode(file_get_contents("php://input"));

    if (isset($objectRequests)) {
        $objectResponses = array();

        foreach ($objectRequests as $objectRequest) {
            $objectResponse = new stdClass();
            $objectResponse->tid = $objectRequest->tid;

            try {
                switch($objectRequest->facadeFn) {

                    // feststellen ob ein Benutzer angemeldet ist
                    case 'idcardcreator.checkLogin':
                        $objectResponse->data = new stdClass();

                        if (isset($this->sUsername)) {
                            $objectResponse->data = array(
                                'username' => $this->sUsername
                            );
                        } else {
                            $objectResponse->data = array(
                                'username' => false
                            );
                        }
                        break;

                    // Liste mit Benutzernamen abrufen
                    case 'idcardcreator.list':
                        $objectResponse->rows = new stdClass();
                        $arrayReturn = $this->_searchADUser(null);

                        if ($arrayReturn instanceof Exception || $arrayReturn instanceof Error) {
                            $this->_writeLog($arrayReturn->getMessage());
                            $objectResponse->errorMsg = $arrayReturn->getMessage();
                        } else {
                            $objectResponse->rows = $arrayReturn;
                        }
                        break;

                    // Formulardaten für AD-Editor holen
                    case 'idcardcreator.loadEdit':
                        $objectResponse->formData = new stdClass();
                        $arrayReturn = $this->_searchADUser($objectRequest->data);

                        if ($arrayReturn instanceof Exception || $arrayReturn instanceof Error) {
                            $this->_writeLog($arrayReturn->getMessage());
                            $objectResponse->errorMsg = $arrayReturn->getMessage();
                        } else {
                            $objectResponse->formData = array(
                                'name' => $arrayReturn[0]['Vorname'] . ' ' . $arrayReturn[0]['Name'],
                                'userid' => $arrayReturn[0]['ID'] !== '--' ? $arrayReturn[0]['ID'] : '',
                                'validity' => $arrayReturn[0]['Gültigkeit'] !== '--' ? $arrayReturn[0]['Gültigkeit'] : '',
                                'position' => $arrayReturn[0]['Funktion'] !== '--' ? $arrayReturn[0]['Funktion'] : ''
                            );
                        }
                        break;

                    // Einloggen
                    case 'idcardcreator.login':
                        $objectResponse->data = new stdClass();
                        $arrayReturn = $this->_loginUser($objectRequest->data->formData);

                        if ($arrayReturn instanceof Exception || $arrayReturn instanceof Error) {
                            $this->_writeLog($arrayReturn->getMessage());
                            if ($arrayReturn->getCode() === 123) {
                                $objectResponse->errorMsg = $arrayReturn->getMessage();
                            } else {
                                $objectResponse->errorMsg = 'Anmeldung fehlgeschlagen.';
                            }
                        } else {
                            $objectResponse->data = array(
                                'success' => 'true'
                            );
                            // Benutzername in Session speichern
                            $_SESSION['username'] = $arrayReturn['username'];
                        }
                        break;

                    // Ausloggen
                    case 'idcardcreator.logout':
                        session_destroy();
                        require_once 'IDCardCreator_ImageManipulator.php'; 
                        $img = new IDCardCreator_ImageManipulator();
                        $img->deleteImgs();
                        break;

                    // Benutzer im ActiveDirectory suchen
                    case 'idcardcreator.search':
                        $objectResponse->rows = new stdClass();
                        $arrayReturn = $this->_searchADUser($objectRequest->data);

                        if ($arrayReturn instanceof Exception || $arrayReturn instanceof Error) {
                            $this->_writeLog($arrayReturn->getMessage());
                            $objectResponse->errorMsg = $arrayReturn->getMessage();
                        } else {
                            $objectResponse->rows = $arrayReturn;
                        }
                        break;

                    // Active Directory benutzer updaten
                    case 'idcardcreator.update':
                        $objectResponse->data = new stdClass();
                        $arrayReturn = $this->_updateADUser($objectRequest->data->formData);

                        if ($arrayReturn instanceof Exception || $arrayReturn instanceof Error) {
                            $this->_writeLog($arrayReturn->getMessage());
                            $objectResponse->errorMsg = $arrayReturn->getMessage();
                        } else {
                            $objectResponse->data = array(
                                'success' => 'true'
                            );
                        }
                        break;

                    default:
                        $objectResponse->errorMsg = 'Aktion nicht gefunden!';
                } 
            } catch (Exception $ex) {
                $this->_writeLog($ex->getMessage());
                $objectResponse->errorMsg = $ex->getMessage();
            }
            $objectResponses[] = $objectResponse;
        }
        // Antwort ausgeben
        print(json_encode($objectResponses));
    } else {
        echo file_get_contents('template/main.html');
    }
}

I would like to reduce the size of this function to increase readability but I'm not sure how to achieve that. I thought of moving the code of each "case" to seperate functions. However my class allready contains 13 functions and I'm afraid it would get to crowded if a added another ~7 functions.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ You might like to lookup the null coalescing operator. \$\endgroup\$ Commented Mar 1, 2019 at 10:05
  • \$\begingroup\$ @mickmackusa what for? \$\endgroup\$ Commented Mar 1, 2019 at 10:08
  • 1
    \$\begingroup\$ because $objectResponse->data = ['username' => $this->sUsername ?? false]; which will reduce the first case by 10 lines \$\endgroup\$ Commented Mar 1, 2019 at 10:23

1 Answer 1

Reset to default
1
\$\begingroup\$

As a matter of semantics, I will urge you to familiarize yourself with the terms of OOP. You might start with https://stackoverflow.com/q/4246883/2943403 , but this is just the first one that I grabbed and not necessarily the best reference -- get a few different perspectives.

As for refactoring your switch block into new/separate methods, yes, I think that is the superior design decision. This will allow you to break up the large block of processing logic into meaningful chunks. In doing so, your script's readability/searchability will improve because you can craft meaningful names for each new method.

On the topic of method naming, run is not a great choice. Perhaps the class name is extremely indicative of what it pertains to, but because classes often increase in size/complexity/etc. over time, it is imperative that you put careful thought into explicit method naming.

I recommended the null coalescing operator in my comment, but if you (and/or your team of devs) are not familiar with this syntax and readabilty is a higher priority then perhaps it would be better to leave your original syntax in place.

I don't recommend blending your printing functionality with your processing functionality. You should always separate this actions into separate calls so that in the future you have greater flexibility. Since you are always generating a string, you can reliably echo this method's return value if you wish.

My personal reference is to write early returns for predictable failures in my scripts versus if (true) { ... a large code block ... } else { a 1 or 2 line fault handler }. This can spare indentation, which can result in less horizontal scrolling (which I have a distaste for).

\$\endgroup\$
6
  • \$\begingroup\$ There are many ways to design your new battery of methods. This is one possible design: 3v4l.org/VJY1O \$\endgroup\$ Commented Mar 2, 2019 at 9:36
  • \$\begingroup\$ It was a "semantic" reference so that you can see the correct wording to use when deiscussing your script. You said that your class already had 13 functions -- they are methods. Yes, I changed my mind about the constructor, after commenting my snippet. I'll edit now. \$\endgroup\$ Commented Mar 4, 2019 at 10:37
  • \$\begingroup\$ @mickmackuse fine, if you want to be pedantic I should call it a method. However, I was refering to it as a function because PHP does not make a difference ==> methods, functions and procedures are all called "function" \$\endgroup\$ Commented Mar 4, 2019 at 10:40
  • \$\begingroup\$ Had to remove my first comment due to en error: I don't see how OOP (with which I'm already familiar with) could help me here. My code is procedural and was always intended to be so. Also, the question you linked has nothing to do with my problem. To include the code from this method into a constructor is a very bad idea since a constructor must not return/print/echo anything! Also, having more than one return per method doesn't seem to be good practice. \$\endgroup\$ Commented Mar 4, 2019 at 10:51
  • \$\begingroup\$ Having one versus multiple returns is debated among professionals. You may maintain that style if you wish; just a suggestion. It feels like you only wish to argue the product of my volunteer time; I'm not sure what you hoped to gain from posting here. \$\endgroup\$ Commented Mar 4, 2019 at 10:55

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.