#319 SMTP full of ugly Copy & Paste

[Anonymous]

It seems that the programmer of the SMTP class (Chris Ryan) has never heard anything of reusable code!

50% of the class is ugly copy and paste !
Why don't you use a function where a function can be used ???

The file is shrinked to 50% if the ugly code is converted into cleanly written code !

Many function can be shrinked to one SINGLE line of code:

public function SendAndMail($from)
{
return $this->SendCmd("SAML", "SAML FROM:$from", 250);
}

public function Verify($name)
{
return $this->SendCmd("VRFY", "VRFY $name", array(250,251));
}

public function Noop()
{
return $this->SendCmd("NOOP", "NOOP", 250);
}

public function StartTLS()
{
if (!$this->SendCmd("STARTTLS", "STARTTLS $extra", 220))
return false;

// Begin encrypted connection
if (!stream_socket_enable_crypto($this->smtp_conn, true, STREAM_CRYPTO_METHOD_TLS_CLIENT))
    return false;

return true;

}

public function Authenticate($username, $password)
{
// Start authentication
if (!$this->SendCmd("AUTH", "AUTH LOGIN", 334))
return false;

if (!$this->SendCmd("Username", base64_encode($username), 334))
    return false;

if (!$this->SendCmd("Password", base64_encode($password), 235))
    return false;

return true;

}

This becomes possible by using this function:

// Send a command and check return code
// $OkCodes may be a single value or an array of allowed return codes
// Writes reply into $this->Reply
private function SendCmd($CMD, $Put, $OkCodes)
{
    if (!$this->Connected()) 
    {
        $this->error = array("error" => "Sending command $CMD without being connected!");
        Log::Error("Smtp ".$this->error["error"]);
        return false;
    }

    fputs($this->smtp_conn, $Put.$this->CRLF);

    $Reply = $this->get_lines();
    $Code  = substr($Reply,0,3);

    if ($this->do_debug)
    {
        $DbgRply = str_replace("\n", "<br>", htmlspecialchars(trim($Reply)));
        echo "Smtp '".htmlspecialchars($Put)."' &rarr; '$DbgRply'";
    }

    if (!in_array($Code, (array)$OkCodes)) 
    {
        $this->Reply = NULL;
        $this->error = array("error"     => "$CMD command not accepted by server.",
                             "smtp_code" => $Code,
                             "detail"    => substr($Reply,4));
        return false;
    }

    $this->Reply = $Reply;      
    $this->error = null;
    return true;
}

The current class is nothing you should be proud about, Chris !

I attach my modified file so you can learn from it.

[Marcus Bointon]

Fixed in 5.2.7 release on github