Facebook "like" remake

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
Trahb
Forum Commoner
Posts: 36
Joined: Sat Jan 30, 2010 9:09 pm

Facebook "like" remake

Post by Trahb »

I did not code the first function, I found it online and it turned out to be pretty useful. My code here is really sloppy, I had to use a TON of comments because of all of the "if" statements. Any suggestions are appreciated :)

Also, the function "userLink" just takes the arg and returns <a href="blahblahpage=ARG">ARG</a>
So it's just a link to their name, displaying their name.

Code: Select all

function array_remove_value() {
	    $args = func_get_args();
	    $arr = $args[0];
	    $values = array_slice($args,1);
	
	    foreach($arr as $k=>$v) {
	        if(in_array($v, $values)) {
	            unset($arr[$k]);
	        }
	    }
	
	    return $arr;
	}

function showLikes($type, $id) {
		$a = mysql_query("SELECT * FROM `skittles_$type` WHERE `id`='$id'") or die(mysql_error());
		$b = mysql_fetch_array($a);
		if ($b['likes'] != NULL) {
			$q = mysql_query("SELECT * FROM `skittles_$type` WHERE `id`='$id'") or die(mysql_error());
			$d = mysql_fetch_array($q);
			$l = explode(" ", $d['likes']);
			$people = count($l);
			$like = "";
			$n = 0;
			$pname = (isset($_SESSION['pname']) ? $_SESSION['pname'] : " ");
			if (in_array($pname, $l)) {
				if ($people == 1) {
					$like .= "You";
				} else if ($people == 2){
					$like .= "You and ";
				} else {
					$like .= "You, ";
				}
			}
			// Now that it has already checked to see if "You" is involved, you can get rid of it
			$l = $this->array_remove_value($l, $pname, "");
			$people = count($l); // Now count how many there are left.
			if ($people > 1) {
				foreach ($l as $name) { // Go through all "likes"
					if ($name != $_SESSION['pname'] && $name != "") {
						$n++;
						$name = $this->userHandler->userLink($name);
						if ($n < $people) {
							// If there are more than two people (and it's not the last name entered) use commas
							if ($people > 2) {
								// If there are MORE THAN 2 and it is on the second to last, add a space
								if ($n == $people) {
									$like .= " ";
								// If it's on anything OTHER THAN the second to last, add a comma
								} else {
									$like .= $name.", ";
								}
							// If there are only 2 people
							} else {
								$like .= $name." ";
							}
						// If $n is equal to the number of people
						} else if ($n == $people) {
							$like .= "and ".$name;
					}
					}
				}
			// If there is only one person that likes it, and it isn't "You"
			} else {
				if ($like != "You") {
					$like .= $this->userHandler->userLink($l[1]);
				}
			}
			$like .= ($people == 1 ? ($like == "You" ? " like this." : " likes this.") : " like this.");
			return $like;
		}
	}
josh
DevNet Master
Posts: 4872
Joined: Wed Feb 11, 2004 3:23 pm
Location: Palm beach, Florida

Re: Facebook "like" remake

Post by josh »

Programmers should wear 2 hats. The builder, and the "refactorer". Once you have your code built (like after each feature you add) you're supposed to go back and "refactor" It would be like writing 10000/20000 as a fraction, which is really just 1/2. You factor it down so you're not taxing the reader's brain. Although as the "architect" you might be thinking in terms of larger things, you should always reduce the code down like this.

Refactorings that need to be applied here
extract method
replace "ifs" with guard clause"
replace "ifs" with polymorphism

This book teaches the lost art: http://www.amazon.com/Refactoring-Impro ... 172&sr=8-1

For example, the comments in your code are describing little chunks of code or what we call kludges of code. Usually the remedy is to take that kludge, and make it its own function altogether (the extract method refactoring). Whatever you wrote in the comment should more or less be the name of the new method. Create the new method and move the kludge of code there. In the place where the kludge used to be, now you only have 1 line of code, that calls the new method.

This way you dont need the comment anymore, and you can place your functions in any order, since youre not relying on top down execution. If you DONT break up your code into functions, the above is what you get, your monolithic code. Its not necessarily bad, but its what we call stove pipe. Its more like hardware than software if you get what I'm saying. Well maintained software is more like working with play doh. Its soft. Well more like a chain than a rope, with a rope if I want to change something I end up unraveling the whole thing. When your program is just a collection of small functions that work together, its like a chain, I can remove any link without affecting the other links.


Although honestly I dont think your code is bad. Its not easy to understand, but as long as it works I'd never need to change it. The real problem with it is you assume I use the same SESSION variables as you, which I do not. You should put your code inside a PHP class, and you should have 'class methods' (really functions) for getting the current user. I can then 'sub-class' your class, and override that method, so I can make it look up the user as I would in my application.
Post Reply