[RESOLVED] Can it be improved?

cluelessPHP

Well I started off with...


<div class="mainBody">
<?php

require "loadclasses.php";
$database = new Database;
			try{

				$sql = "SELECT count(*) FROM username WHERE activecode = :activecode"; 	
				$stmt = $database->query($sql);

				$database->execute();
				$database->rowCount();
				 if ($database->rowCount() === 1){
					echo "Your account is now active";
							header( "refresh:10; url=index.php" ); 
						echo "<br><a href='profilesetup.php'>Home page</a>";
					$completeActive = "UPDATE username SET activecode = 'Active', active = '0'  WHERE activecode = :activecode"; 

					$setActive = $database->query($completeActive);
					$setActive->bindParam(':activecode',  $_GET["activecode"]);
					$setActive->execute();
				}
				else {
					echo "Your account is already active";
					header( "refresh:10; url=index.php" ); 
						echo "<br><a href='index.php'>Home page</a>";
						exit;
				}
		}
			catch(Exception $e ) {
				echo "Your account is already active";
				header( "refresh:10; url=login.php" ); 
						echo "<br><a href='login.php'>Login page</a>";
						exit;
        }
	?>
</div>

Now I have

function makeActive()
{
	try{
					$database = new Database;
					$text = "";
					$activecode = $_GET["activecode"];

				$setActive = "UPDATE username SET active = '1', activecode = 'Active' WHERE activecode = :activecode";				
				$stmt = $database->query($setActive);
				$database->bind(':activecode', $activecode);
				$database->execute();
				$database->rowCount();
				if ($database->rowCount() === 1)
				{
					return $text .="Your account is now active" . "<br><a href='index.php'>Home page</a>";
					header( "refresh:10; url=index.php" ); 

				}
				else
				{
					return $text .="Your account is already active" . "<br><a href='index.php'>Home page</a>";

					header( "refresh:10; url=index.php" ); 

				}
	}
			catch(Exception $e ) 
			{
				return $text .= "Something went wrong contact site administrator" . "<br><a href='index.php'>Login page</a>";
				header( "refresh:10; url=index.php" ); 
      		}
}

Seems reasonable?

cluelessPHP

Yes it can be it's bugged :glare:

laserlight

In makeActive, you are returning $text (right after appending to it) before calling header, and then your header call has a hardcoded time and URL. Since you are going to make the same header call no matter which branch the flow of control goes to, I suggest:

function makeActive()
{
    try
    {
        $database = new Database;
        $activecode = $_GET["activecode"];
        $setActive = "UPDATE username SET active = '1', activecode = 'Active' WHERE activecode = :activecode";
        $stmt = $database->query($setActive);
        $database->bind(':activecode', $activecode);
        $database->execute();
        $rowCount = $database->rowCount();
    }
    catch (Exception $e)
    {
        return "Something went wrong contact site administrator<br><a href='index.php'>Login page</a>";
    }

if ($rowCount === 1)
{
    return "Your account is now active<br><a href='index.php'>Home page</a>";
}
else
{
    return "Your account is already active<br><a href='index.php'>Home page</a>";
}
}

then you call makeActive followed by header.

cluelessPHP

Oh so it would be

<div class="mainBody">
<?php

echo makeActive();
    header( "refresh:10; url=index.php" );  
?>
</div>

Or that idea?

NogDog

Okay, spent some time turning this into a refactoring tutorial of sorts. 🙂

[EDIT]Just noticed that you used $database in a couple places where you should have used $stmt. Editing it here in my re-factored code section....[/EDIT]

First impressions:

function makeActive()
{
    try {  // I would not try/catch within a function, let the app do that
        $database = new Database; // I would pass this in, rather than depend on it
        $text = "";  // this variable serves no purpose
        $activecode = $_GET["activecode"]; // I would pass this in, rather than depend on it

    $setActive = "UPDATE username SET active = '1', activecode = 'Active' WHERE activecode = :activecode";
    $stmt = $database->query($setActive);
    $database->bind(':activecode', $activecode);
    $database->execute();
    $database->rowCount(); // no need to do this twice
    // in the if/else, you are doing HTML output from a makeActive() function, meaning
    // the function is doing more than that. Instead, I would just return a result indicator
    // that the main app (or other output function) could use to inform the user of the result
    // as desired. Keep your functions narrowly focused when possible.
    if ($database->rowCount() === 1) {
        return $text .="Your account is now active" . "<br><a href='index.php'>Home page</a>";
        header("refresh:10; url=index.php");
    } else {
        return $text .="Your account is already active" . "<br><a href='index.php'>Home page</a>";                 
        header("refresh:10; url=index.php");
    }
} catch (Exception $e) {
    return $text .= "Something went wrong contact site administrator" . "<br><a href='index.php'>Login page</a>";
    header("refresh:10; url=index.php");
}
}

My initial pass at re-factoring it to something more like I would prefer, to keep the function focused only on its job of activating a user as well as generally DRYing things up:

/**
 * Activate a user
 *
 * @param PDO $database
 * @param string $activatecode
 * @return Boolean true: successfully activated, false: already activated
 */
function makeActive(PDO $database, $activatecode)
{
    $setActive = "UPDATE username SET active = '1', activecode = 'Active' WHERE activecode = :activecode";
    $stmt = $database->query($setActive);
    $stmt->bindParam(':activecode', $activecode); // that's the method you really want (assuming PDO?)
    $stmt->execute();
    return (bool) $stmt->rowCount();
}

// app code...
try {
    $db = new Database;
    if (makeActive($db, $_GET['activecode'])) {
        echo "Your account is now active";
    }
    else {
        echo "Your account is already active";
    }
    echo "<br><a href='index.php'>Home page</a>" // D.R.Y.!
}
catch(PDOException) {
    // whatever you want to do if you get a DB error
}
catch(Exception) {
    // whatever you want to do with other exceptions
}
finally {
    header("refresh:10; url=index.php");
}

NogDog

PS: I was assuming your Database class extends PDO. If not, then you'll need to change the function's type-hint for the first argument to be Database instead of PDO -- though I might try to convince you otherwise. 😉

cluelessPHP

Oh my database class looks like...


<?php
date_default_timezone_set("Europe/London");


error_reporting(-1); // reports all errors
ini_set("display_errors", "1"); // shows all errors
ini_set("log_errors", 1);
ini_set("error_log", "php-error.log");

class Database
{
	private $host = "localhost";
	private $user = "root";
	private $pass = "";
	private $dbname = "got";

private $dbh;
private $error;
private $stmt;

public function __construct()
{
	//SET DSN
	$dsn = "mysql:host=". $this->host . ";dbname=" . $this->dbname;

	$options = array
	(
		PDO::ATTR_PERSISTENT 	=> true,
		PDO::ATTR_ERRMODE 		=>PDO::ERRMODE_EXCEPTION,
		PDO::MYSQL_ATTR_FOUND_ROWS => true
	);
//create PDO
try
{
	$this->dbh = new PDO($dsn, $this->user, $this->pass, $options);
}
catch(PDOEception $e)
{
	$this->error = $e->getMessage();
}
}

public function query($query)
{
	$this->stmt = $this->dbh->prepare($query);
}

public function bind($param, $value, $type = null)
{
	if(is_null($type))
	{
		switch(true)
		{
			case is_int($value):
			$type = PDO::PARAM_INT;
			break;
			case is_bool($value):
			$type = PDO::PARAM_BOOL;
			break;
			case is_null($value):
			$type = PDO::PARAM_NULL;
			break;
			default:
			$type = PDO::PARAM_STR;
		}
	}

	$this->stmt->bindValue($param, $value, $type);
}

public function execute()
{
	return $this->stmt->execute();
}

public function lastInsertId()
{
	$this->dbh->lastInsertId();
}
public function rowCount()
{
	return $this->stmt->rowCount();
}

public function connect()
{
	$this->dbh->connect();
}

public function resultset()
{
	$this->execute();
	return $this->stmt->fetchAll(PDO::FETCH_ASSOC);
}
}

Both examples are great

cluelessPHP

Catchable fatal error: Argument 1 passed to makeActive() must be an instance of PDO, instance of Database given, called in C:\Program Files (x86)\EasyPHP-Devserver-17\eds-www\gotsocial\functions.php on line 17 and defined in C:\Program Files (x86)\EasyPHP-Devserver-17\eds-www\gotsocial\functions.php on line 5

Er I don't know how to approach that one

NogDog

Change the function definition to use a type-hint of [FONT=Courier New]Database[/FONT] instead of [FONT=Courier New]PDO[/FONT].

(Personally I'd rather either just use PDO, or have my database class extend PDO -- but that's just me. 🙂 )

cluelessPHP

Well i read this thing here

http://radify.io/blog/type-hinting-in-php-good-or-bad-practice/

And tried this


function makeActive(Database $database, $activatecode) 
{ 
    $setActive = "UPDATE username SET active = '1', activecode = 'Active' WHERE activecode = :activecode"; 
    $stmt = $database->query($setActive); 
    $stmt->bindParam(':activecode', $activecode); // that's the method you really want (assuming PDO?) 
    $stmt->execute(); 
    return (bool) $stmt->rowCount(); 
} 

// app code... 
try { 
    $db = new Database; 
    if (makeActive($db, $_GET['activecode'])) { 
        echo "Your account is now active"; 
    } 
    else { 
        echo "Your account is already active"; 
    } 
    echo "<br><a href='index.php'>Home page</a>"; // D.R.Y.! 
} 
catch(PDOException $e) { 
    // whatever you want to do if you get a DB error 
    echo "Something went wrong contact site administrator";
} 
catch(Exception $e) { 
    // whatever you want to do with other exceptions 
     echo "Something went wrong contact site administrator";
} 
finally { 
    header("refresh:10; url=index.php"); 
}

Fatal error: Call to a member function bindParam() on null

I guess the Database class is wrong?

cluelessPHP

Oh I did try

class Database extends PDO

Same error, for whatever reason var_export()/ var_export() won't show anything only the message

Fatal error: Call to a member function bindParam() on null in C:\Program Files (x86)\EasyPHP-Devserver-17\eds-www\gotsocial\functions.php on line 14

NogDog

    $stmt = $database->query($setActive);

...should probably be...

    $stmt = $database->prepare($setActive);

...?

Unless your Database class uses query() to actually do a prepare()? (Which would be pretty confusing 😉 )

[edit]Oh, I see you did do that, so, umm..., never mind?[/edit]

NogDog

PS: In any case, I probably had you changing too many things at once (on top of some incorrect assumptions on my part). Try incrementally changing things, perhaps, so you know which changes broke things when you test it. W.r.t. type-hinting, even if your Database class does extend PDO, if your function requires some aspect of the Database class that's different or in addition to what's in PDO, then you'll want to type-hint for Database anyway (like if you want to use your bind() method instead of the vanilla bindParam().

cluelessPHP

Oh, I see, it makes sense now I see it.

Yes, why do it the easy way when you can erm...confuse yourself? :rolleyes:

I really should get used to the correct terms rather than "that thing you do when you want to connect to a database"