Retrieving data from active directory with PHP

Question

I have the following php function, that reads and returns data from the active directory. It is part of a project which will be graded (I'm in an apprenticeship). It is very long and I think the code would be much more readable if I broke it down to several small functions. How could I do that?

private function _searchADUser($filters) {
    try {
        if ($filters !== null) {
            $sn = utf8_decode($filters->name);
            $givenName = utf8_decode($filters->firstName);
            $employeeId = $filters->id;
        } else {
            $sn = '';
            $givenName = '*';
            $employeeId = '';
        }
        $pattern = '(&';

        if ($givenName !== '') {
            $pattern .= '(givenName='.$givenName.')';
        }

        if ($sn !== '') {
            $pattern .= '(sn='.$sn.')';
        }

        if ($employeeId !== '') {
            $pattern .= '(employeeid='.$employeeId.')';
        }

        $pattern .= ')';

        $con = ldap_connect($this->ldap->connection);
        ldap_bind($con, $this->ldap->username, $this->ldap->password);

        $result = ldap_search($con, $this->ldap->dn, $pattern);
        $userInfo = ldap_get_entries($con, $result);

        if ($userInfo['count'] === 0) {
            throw new ErrorException('Die Suche lieferte keine Ergebnisse.');
        }

        $return = [];

        for ($i = 0; $i < $userInfo['count']; $i++) {
            if (array_key_exists('sn', $userInfo[$i])) {
                $name = $userInfo[$i]['sn'][0];
            } else {
                $name = '--';
            }
            if (array_key_exists('givenname', $userInfo[$i])) {
                $vorname = $userInfo[$i]['givenname'][0];
            } else {
                $vorname = '--';
            }               

            // Ausweisnummer auslesen
            if (isset($userInfo[$i]['employeeid'])) {
                $ausweisnr = $userInfo[$i]['employeeid'][0];
            } else {
                $ausweisnr = '--';
            }

            // Gültigkeitsdatum auslesen
            if (isset($userInfo[$i]['accountexpires'])) {
                if($userInfo[$i]['accountexpires'][0] !== '0' && $userInfo[$i]['accountexpires'][0] !== '9223372036854775807') {
                    $accExp = floatval($userInfo[$i]['accountexpires'][0]);
                    $floatDate = $accExp/1.E7-11644473600;
                    $intDate = intval($floatDate);
                    $valid = date('d.m.Y', $intDate);
                } else {
                    // 31. Dezember des laufenden Jahres wenn kein Datum gesetzt
                    $valid = date('d.m.Y', strtotime('12/31'));
                }
            } else {
                $valid = date('d.m.Y', strtotime('12/31'));
            }

            // Funktion auslesen
            if (isset($userInfo[$i]['title'][0])) {
                $title = $userInfo[$i]['title'][0];
            } else {
                $title = '--';
            }

            // Benutzername auslesen
            if (isset($userInfo[$i]['samaccountname'][0])) {
                $username = $userInfo[$i]['samaccountname'][0];
            } else {
                $username = '--';
            }

            require_once 'PHP/IDCardCreator_ImageManipulator.php';
            $img = new IDCardCreator_ImageManipulator();

            // Anzeigebild auslesen
            if (isset($this->ldap->picturepath) && isset($userInfo[$i]['samaccountname'][0])) {
                if (is_file($this->ldap->picturepath.'\\'.$userInfo[$i]['samaccountname'][0].'.jpg')) {
                    copy($this->ldap->picturepath.'\\'.$userInfo[$i]['samaccountname'][0].'.jpg', 'userImages/'.$userInfo[$i]['samaccountname'][0].'.jpg');
                    $path = 'userImages/'.$userInfo[$i]['samaccountname'][0].'.jpg';
                } else {
                    // Pfad des Platzhalterbildes übergeben
                    $this->_writeLog('Datei '.$this->ldap->picturepath.'\\'.$userInfo[$i]['samaccountname'][0].'.jpg nicht gefunden.');
                    $path = 'img/noimg.png';
                }
            } else {
                if (isset($userInfo[$i]['thumbnailphoto']) && $filters !== null) {
                    $imgString = $userInfo[$i]['thumbnailphoto'][0];
                    $img->saveImg($name, $vorname, $imgString);
                    // Pfad des Bildes übergeben
                    $path = 'userImages/'.$name . '_' . $vorname . '.jpg';
                } else {
                    // Pfad des Platzhalterbildes übergeben
                    $path = 'img/noimg.png';
                }
            }

            $results = array(
                // Umlaute korrekt codieren
                'Name' => utf8_encode($name),
                'Vorname' => utf8_encode($vorname),
                'Funktion' => utf8_encode($title),
                'Gültigkeit' => $valid,
                'ID' => $ausweisnr,
                'Pfad' => utf8_encode($path)
            );
            $return[] = $results;
        }

        // Array alphabetisch sortieren
        usort($return, function($a, $b) {
            return $a['Name'] < $b['Name'] ? -1 : 1;
        });
    } catch (Throwable $ex) {
        $return = $ex;
    }
    return $return;
}

Answer 1

There certainly are ways to break this down to more understandable chunks, in fact, there are refactoring techniques that address your specific issue, long method.

You can look at https://refactoring.guru/smells/long-method

The most common way to simplify a long method is just to break it up to smaller parts using the Extract method technique.

So for example, first, I would take out the pattern creation and make an own method for that (I'm assuming you are working in a class):

/**
 * @param $filters
 * @return string
 */
private function getPattern($filters): string
{
    if ($filters !== null) {
        $sn = utf8_decode($filters->name);
        $givenName = utf8_decode($filters->firstName);
        $employeeId = $filters->id;
    } else {
        $sn = '';
        $givenName = '*';
        $employeeId = '';
    }
    $pattern = '(&';

    if ($givenName !== '') {
        $pattern .= '(givenName=' . $givenName . ')';
    }

    if ($sn !== '') {
        $pattern .= '(sn=' . $sn . ')';
    }

    if ($employeeId !== '') {
        $pattern .= '(employeeid=' . $employeeId . ')';
    }

    $pattern .= ')';
    return $pattern;
}

private function _searchADUser($filters) {
    try {
        $pattern = $this->getPattern($filters);

        $con = ldap_connect($this->ldap->connection);
        ...

You can refactor the getPattern method further but let's wait with that now and see what else we can do.

The other chunk would probably be to take out that ldap stuff and put that in it's own method:

/**
 * @param string $pattern
 * @return array
 * @throws ErrorException
 */
private function getUserInfo(string $pattern): array
{
    $con = ldap_connect($this->ldap->connection);
    ldap_bind($con, $this->ldap->username, $this->ldap->password);

    $result = ldap_search($con, $this->ldap->dn, $pattern);
    $userInfo = ldap_get_entries($con, $result);

    if ($userInfo['count'] === 0) {
        throw new ErrorException('Die Suche lieferte keine Ergebnisse.');
    }
    return $userInfo;
}

The main method know looks something like this:

private function _searchADUser($filters) {
    try {
        $pattern = $this->getPattern($filters);
        $userInfo = $this->getUserInfo($pattern);

When refactoring, I like to try to make the code as clear as I can and one thing that makes code difficult to read is when the naming changes, my advice would be to stick be the name already in use.

$name = $userInfo[$i]['sn'][0];

would become

$sn = $userInfo[$i]['sn'][0];

Break them out in their own methods:

private function getSN(array $userInfo, int $i): string
{
    if (array_key_exists('sn', $userInfo[$i])) {
        $sn = $userInfo[$i]['sn'][0];
    } else {
        $sn = '--';
    }
    return $sn;
}

private function getGivenName(array $userInfo, int $i): string
{
    if (array_key_exists('givenname', $userInfo[$i])) {
        $givenname = $userInfo[$i]['givenname'][0];
    } else {
        $givenname = '--';
    }
    return $givenname;
}

private function getEmployeeId(array $userInfo, int $i): string
{
    if (isset($userInfo[$i]['employeeid'])) {
        $employeeId = $userInfo[$i]['employeeid'][0];
    } else {
        $employeeId = '--';
    }

    return $employeeId;
}

The for loop would then start like this:

for ($i = 0; $i < $userInfo['count']; $i++) {
    $sn = $this->getSN($userInfo, $i);
    $givenName = $this->getGivenName($userInfo, $i);
    $employeeId = $this->getEmployeeId($userInfo, $i);

As you can see, you can break up the code into smaller chunks and by doing so making it more approachable.

I will stop here but please continue to explore the extract method technique and continue to apply it.

Hopes this helps.