Feedback about my code

Olli · February 5, 2012

I'm just learning MySQL and PDO. Could you see my code and give some feedback? Example what I could do better in other way.

<?php

try {
    $yhteys = new PDO("mysql:host=localhost;dbname=****", "******", "***********");
} catch (PDOException $e) {
    die("VIRHE: " . $e->getMessage());
}
// virheenkäsittely: virheet aiheuttavat poikkeuksen
$yhteys->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

function dbAll($query, $data){
global $yhteys;
$kysely = $yhteys->prepare($query);
$kysely->execute($data);
return $kysely->fetchAll();
}

function fetchRow($id){
global $yhteys;
$kysely = $yhteys->prepare("SELECT * FROM kampanjat WHERE id = ? LIMIT 1");
$kysely->execute(array($id));
return $kysely->fetch();
}

function action($action, $id, $text){

switch($action){
	case "edit":
	$url = "?a=edit&id=$id";
	break;

	case "view":
	$url = "?a=view&id=$id";
	break;

	case "delete":
	$rivi = fetchRow($id);
	$nimi = $rivi["nimi"];

	$url = "#";
	$onclick = "javascript:delete('{$nimi}', '{$id}');";
	break;

}

//$return = "<a href='javascipt:window.open(\"index.php{$url}\");'";

$return = "<a href='#'";

/*if($onclick){*/ $return .= " onclick='popUp(\"index.php{$url}\");{$onclick}'"; /*}*/

$return .= ">{$text}</a>";

return $return;

}

function ico($name){
return "<img src='icons/{$name}.png'>";
}

function initHeader(){
?>

<html>
<head>
<title> Kokoelma</title>

<link rel="stylesheet" type="text/css" href="style.css" />
<script type="text/javascript" src="js.js"></script>

</head>

<body>

<?php
} 
?>

Thank you very much

trq · February 5, 2012

Three things. Indent your code properly, don't use the global keyword and name your variables something more descriptive.

Olli · February 5, 2012

Thank you for your comments.

I will do the indention soon, but could you say, what I should use instead of global?

trq · February 5, 2012

Functions accept arguments. If you need something from outside a function within a function you should pass it in. You completely break reusability otherwise.

Olli · February 5, 2012

Ok, could you give example how I should actually do it?

trq · February 5, 2012

Just pass $yhteys in as an argument. And while your there, name it something like $pdo, it is more meaningful than $yhteys.

Olli · February 5, 2012

Ok, but what is an argument ?

trq · February 5, 2012

Your already using them. For instance, to pass an $id into your fetchRow function.

Olli · February 5, 2012

Yes, but I still cannot understand why should I pass PDO as function argument and how?

trq · February 5, 2012

Why:

Because currently your functions relie on the variable $yhteys existing outside of the function. This means you cannot reliably reuse these functions in some other project.

How:

function fetchRow($pdo, $id) {
    $kysely = $pdo->prepare("SELECT * FROM kampanjat WHERE id = ? LIMIT 1");
    $kysely->execute(array($id));
    return $kysely->fetch();
}

Looking closer at your functions they aren't reusable anyway because they also relie on a specific table existing in the database.

Olli · February 5, 2012

OK.Actually I think I won't reuse the functions other projects, so it's ok now.

Here's my currently code, do you have anything else that I could do better?

Thinking about dbOne and dbAll functions, are they too complex to use?

About your last things:

"Indent your code properly," -> I tried to indent it now, OK?

"name your variables something more descriptive" -> their names are in Finnish, so I can understand them

Thank you for your comments already.

<?php

$action = $_GET["a"];
$id		= $_GET["id"];

try {
    $yhteys = new PDO("mysql:host=localhost;dbname=****", "******", "*******");
} catch (PDOException $e) {
    die("VIRHE: " . $e->getMessage());
}

$yhteys->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

function dbOne($query, $data=array(), $return="array"){
global $yhteys;
$kysely = $yhteys->prepare($query);
$kysely->execute($data);
if($return=="array"){return $kysely->fetch();}else{return $kysely;}
}

function dbAll($query, $data=array(), $return="array"){
global $yhteys;
$kysely = $yhteys->prepare($query);
$kysely->execute($data);
if($return=="array"){return $kysely->fetchAll();}else{return $kysely;}
}

function dbRow($query){
return $query->rowCount();
}

function promoType($id){
$getpromo = dbOne("SELECT * FROM tyypit WHERE id = ?", array($id));
return $getpromo["nimi"];
}

function action($action, $id, $text){

	switch($action){
		case "edit":
		$url = "?a=edit&id=$id";
		break;

		case "view":
		$url = "?a=view&id=$id";
		break;

		case "delete":
		$rivi = fetchRow($id);
		$nimi = $rivi["nimi"];
		$url = "#";
		$onclick = "javascript:delete_entry(\"{$nimi}\", \"{$id}\");";
		break;
	}

$return = "<a"; 

	if($onclick) {
	$return .= " href='#' onclick='{$onclick}'";
	} else {
	$return .= " href='#' onclick='popUp(\"index.php{$url}\");{$onclick}'";
	}
  
$return .= ">{$text}</a>";

return $return;

}

function ico($name){
return "<img src='icons/{$name}.png'>";
}

function promoLink($thisid, $name){
global $id;
	if($id == $thisid){
	return"<span class='selected'>{$name}</span>";
	} else {
	return"<a href='index.php?id={$thisid}'>{$name}</a>";
	}
}

function sqldate($phpdate){
return date('Y-m-d H:i:s', $phpdate);
}

function phpdate($mysqldate){
return strtotime($mysqldate);
}	

function initHeader(){
?>

<html>
<head>
<title> Kokoelma</title>

<link rel="stylesheet" type="text/css" href="style.css" />
<link rel='stylesheet' type='text/css' href='http://fonts.googleapis.com/css?family=Open+Sans'>

<script type="text/javascript" src="js.js"></script>

</head>

<body>

<?php
} 
?>

trq · February 5, 2012

Using globals in a function is never a good idea and you wouldn't want to pickup a bad habit. Functions have there own scope for a reason.

Olli · February 5, 2012

Yes,but code is more hard to read if functions have too much of parameters

trq · February 5, 2012

No it's not. It actually makes more sense because you know where the variables are coming from.

Using globals, you have some variable magically appearing within your function.

Sign In

Feedback about my code

Recommended Posts

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Join the conversation

Important Information