Updating my old code

[Derokorian]

So the first site I made for someone else has been up for about 2 years now, and they want me to change it so the pictures of kids can be viewed in a modal dialog instead of a mouse over pop-up that I found. I thought easy, no problem I'll just use jQuery I'm starting to get better with it. Then I opened up my FTP and looked at old code, when I found the pictures were displayed with a function (oh I functionalized something, but horribly horribly wrong) I looked up the function and this is what I found:

function picBar($id) {
    GLOBAL $conn;
    GLOBAL $siteurl;
    GLOBAL $dbkidpics;
    GLOBAL $dbkids;
    GLOBAL $sitepath;
    GLOBAL $kidsimg;
    $picqry = "SELECT pics_file,kids_name FROM {$dbkidpics} JOIN {$dbkids} ON pics_kid=idwaiting_kids WHERE pics_kid={$id} AND pics_profile=0 ORDER BY pics_date DESC LIMIT 5";
    $picresult = mysql_query($picqry,$conn);
    $pic = "<div id=\"picbar\"><span class=\"hint\">Mouse over to view larger</span><br />";
    while ( $row = mysql_fetch_array( $picresult ) ) {
        $image = "{$sitepath}{$kidsimg}{$row['pics_file']}";
        $imagesize = getimagesize($image);
        $imagewidth = $imagesize[0];
        $imageheight = $imagesize[1];
        $popupwidth = $imagesize[0];
        $popupheight = $imagesize[1];
		  $pic .= "<a href=\"javascript:void(0)\" onmouseover=\"return overlib('&lt;img src={$siteurl}{$kidsimg}{$row['pics_file']} width={$imagewidth} height={$imageheight} /&gt;', FGCOLOR, '#DDEEEE', VAUTO, RIGHT);\" onmouseout=\"return nd();\">\n";
        $pic .= "<img src=\"{$siteurl}{$kidsimg}{$row['pics_file']}\" alt=\"{$row['kids_name']}\" width=\"87px\" border=\"0\" /></a><br />\n";
    }
    $pic .= "</div>";
    return $pic;
}

And I started shuddering. I can't believe I relied so heavily on globals in this project. It makes me want to redo the entire site from the ground up... I mean seriously look at all those globals! Not to mention creating variables I never use, not varifying the return of the query... This code makes me realize just how far I've come, and yet it kind of makes me sick that I wrote this!!

In fact looking at this now I don't even see a quick solution to implementing Lightbox or my own jQuery modal view... It just so bad lol..

Thought you all might enjoy my horrible horrible code! XD

[dalecosp]

Oh, I have certainly seen worse. One of my first projects is ten years old any day now, and the owner (who has little contact with me, although I count him a friend) says he loves it, it puts him first on Google results for queries that relate to his business, etc., except he can't change anything on it and shipping costs a lot more now than it did in 2002 and he recently lost money when someone pointed out the old pricing.

IIRC, it's line after line of:

echo "This is a line of text about ";
echo $foo;
echo " and the reason why you need to buy it.";

Plus very 'infantile' calculations, unnecessary vars, etc., etc.

I'm hoping to create a control panel for him without touching the old code ... but you know that can't happen.

Being of sound mind and body, if the worst DOES occur, I will to you my avatar and post count. :p

[Derokorian]

Oh god it gets worse...

function sqlSafe($input) {
    GLOBAL $db;
    //Replace quotes and other characters, with HTML and SQL safe code
    $replacethis = array('“','”','‘','’','—','…',);
    $replacewith = array('"','"',''',''','-','…');
    $return = str_replace($replacethis,$replacewith,$input);
    return $db->real_escape_string(trim($return));
}

I'm so glad y'all taught me to not be a complete idiot XD

[Bonesnap]

I once wrote a function that had an optional boolean parameter (default was true). If the parameter was false the function didn't run.

I still wonder to this day why I did that. :o

[dalecosp]

Bonesnap;10995345 wrote:

I once wrote a function that had an optional boolean parameter (default was true). If the parameter was false the function didn't run.

I still wonder to this day why I did that. :o

Perfect! That one deserves a medal --- or a paragraph in a book. Wonderful! 🙂

[Bonesnap]

dalecosp;10995761 wrote:

Perfect! That one deserves a medal --- or a paragraph in a book. Wonderful! 🙂

Haha yeah it's probably one of the weirdest things I've ever written.

[Derokorian]

I still don't understand..

function foo($arg,$run) {
   if( !$run ) return;
   echo $arg;
}

?????

[Bonesnap]

Derokorian;10995778 wrote:

I still don't understand..

function foo($arg,$run) {
   if( !$run ) return;
   echo $arg;
}

?????

Essentially yes, except $run is true by default.

function foo($arg, $run=true)
{
    if(!$run) return;
    //something else
}

[Derokorian]

Well I can't explain why you did that either!

[dalecosp]

Ah ... here's what I had pictured:

function wibble($blat=null) {
  if (!$blat) do_something();
}

Also pretty strange, eh?

[Bonesnap]

I looked through several old projects to see if I could find the actual function but no such luck. I probably deleted it out of embarrassment. 🙂

[Derokorian]

So I'm finally out of the weeds on all projects here at work and decided before I got swamped again I would try to start refactoring and TBH I have no idea where to start... Its just so bad - little to no comments, no separation of logic and presentation, globals in functions everywhere, queries in loops that are results of another query (and even once this goes 3 deep)... Other code that I've refactored started in a much better state than this and I feel overwhelmed at the idea of fixing it.

[Bonesnap]

With code that "bad", I find rewriting it from scratch is usually the best solution. At least for me.

If you're like me, and probably other developers, your knowledge of PHP, HTML, JavaScript, and web development in general has increased substantially compared to what you knew two years ago, and refactoring the code may be more work than rewriting it.

[Derokorian]

Well what's sad is its been less than a year since I finished it, but yeah my knowledge has come so far since then. I didn't even belong to this forum when I made the site in question heh and I can tell you belonging here has made such a HUGE difference in my competency level.

[Derokorian]

No comment about how bad this is:

date_default_timezone_set('EST');
$title = "Classes";
$keywords = ",pre-service,continuin ed,classes,education,support";
$groupsql = "SELECT
		  *
	 FROM $dbcgroup
	 INNER JOIN $dbaloc
		  ON id_location=class_location
	 ORDER BY class_reg_deadline ASC";
$groupresult = mysql_query( $groupsql, $conn ) or DIE("Database Error!<br />". mysql_error() );
$content .= "<h1 class=\"center\">Class Schedules</h1><p style=\"text-indent:0px;margin-left:15px;\">Adoption and foster care pre-service classes are filled on a first-come, first-served basis.</p>\n";
$content .= "<strong>To Register:</strong> &nbsp;Call John Hostetler at 1-800-686-6627, ext. 123<br /><br />";
$content .= "<div style=\"height:10px;clear:both;\"></div>";
while( $group = mysql_fetch_array($groupresult) ) {
	 $content .= "<table width=\"675px\">";
	 $content .= "<tr><td colspan=\"2\"><a name=\"{$group['id_class_group']}\"></a><h1 class=\"classgroup\">{$group['class_group_name']}</h1>\n";
	 $content .= "<h4>Registration Deadline: &nbsp;".date("M d, Y",$group['class_reg_deadline'])."</h4>\n";
	 $content .= "<h4>Location: &nbsp;{$group['loc_title']} office</h4></td></tr>\n";
	 $daysql = "SELECT
				*
		  FROM $dbcday
		  WHERE class_group={$group['id_class_group']}
		  ORDER BY class_day_start ASC";
	 $dayresult = mysql_query( $daysql, $conn ) or DIE("Database Error!<br />". mysql_error() );
	 while( $day = mysql_fetch_array($dayresult) ) {
		  $topicsql = "SELECT
					 IF(`class_title`='Lunch',CONCAT('<i>',`class_title`,'</i>'),IF(`class_title`='Dinner',CONCAT('<i>',`class_title`,'</i>'),`class_title`)) as `title`,
					 class_topic . * ,
					 class_topic_title . *
				FROM $dbctopic
				INNER JOIN {$dbctitle}
					 ON id_class_title=class_name
				WHERE class_group={$group['id_class_group']}
					 AND class_day={$day['id_class_day']}
				ORDER BY class_start_time ASC";
		  $topicresult = mysql_query( $topicsql, $conn ) or DIE("Database Error!<br />". mysql_errno() .' '. mysql_error() . '<br />' .$topicsql);
		  $content .= "<tr><td width=\"125px\"><h2 class=\"classday\">".date("M j",$day['class_day_start'])."</h2>\n".( (date("i",$day['class_day_start'])=='00') ? date("g a",$day['class_day_start']) : date("g:i a",$day['class_day_start']) )." to ".( (date("i",$day['class_day_end'])=='00') ? date("g a",$day['class_day_end']) : date("g:i a",$day['class_day_end']) )."</td><td><table>";
		  while ( $topic = mysql_fetch_array($topicresult) ) {
				$content .= "<tr><td width=\"100px\">".( (date("i",$topic['class_start_time'])=='00') ? date("g a",$topic['class_start_time']) : date("g:i a",$topic['class_start_time']) )." - ".( (date("i",$topic['class_end_time'])=='00') ? date("g a",$topic['class_end_time']) : date("g:i a",$topic['class_end_time']) )."</td><td>";
				$content .= "{$topic['title']}";
				$content .= "</td></tr>";
		  }
		  $content .= "</table></td></tr>";
	 }
	 $content .= "</table><div style=\"height:25px;\"></div><div style=\"page-break-after:always\"></div>";
}
$content = substr($content,0,-43);

[Bonesnap]

Derokorian;10998007 wrote:

No comment about how bad this is:

'continuin ed'

I hope it's still continuing. 😉

Haha I also love that the die statement is in uppercase. I'm going to do that from now on.

[Derokorian]

Bonesnap;10998049 wrote:

I hope it's still continuing. 😉

Haha I also love that the die statement is in uppercase. I'm going to do that from now on.

Aha I didn't even see that!

[NogDog]

I don't know why, but this is my favorite part:

$content = substr($content,0,-43); 

I think it's because I imagine the time wasted until you figured out that the two back-slashes in the preceding command weren't in the actual string you wanted to trim off of the end. 🙂

[spufi]

Derokorian;10998007 wrote:

No comment about how bad this is:

Code...

This was me before I saw some code done from a MVC point of view that made it click for me. I shake my head at seeing PHP generating markup at this point.