14 Replies - 3565 Views - Last Post: 07 January 2013 - 09:14 PM Rate Topic: -----

#1 Jstall  Icon User is offline

  • Lurker
  • member icon

Reputation: 434
  • View blog
  • Posts: 1,042
  • Joined: 08-March 09

Awful PHP

Posted 21 October 2012 - 01:33 PM

I was to preface this by saying I don't think I'm the best programmer in the world or anything, I'm not the type of person that goes around bashing other programmers work. I would much rather make suggestions on how one can improve their code, just as I am constantly being shown how to improve my work by the people here at D.I.C and the people I work with. That being said.. I just had to share this.

Ok so I have a side gig where I work as a contractor with my former full time employer. The company recently attained the services of a small local web development company to do some work on the project I work on. After the local company committed their stuff to source control I was asked to do a code review of their stuff to get an idea of their competency level. This is what I found.

First of all they were using deprecated mysql* functions for database access. Ok, not a big deal, even though the project has a Database class to do this stuff. I can see maybe they were not aware of this. Then I notice none of the incoming data is escaped in any way. It's wide open to SQL injection and XSS in literally dozens of places. This is when I started to get worried.

The site has members, people can register using Facebook in which case their Facebook id is stored along with their other account information. One of the things this company had to do was check if any of the logged in members Facebook friends were members of the site identified by the Facebook id we store. This is what I saw:

$users; 
$friendsList = $facebook->api($fbProfile.'/friends');
$user_fb_uid;
$sql = "SELECT `member_id`, `facebook_uid` FROM `members`";

$result = mysql_query($sql);
   
  if($result){
       while($data = mysql_fetch_object($result)) {
       	
            $users[] = $data->member_id;
            $user_fb_uid[] = $data->facebook_uid;           
         }
  }
$bpFriends = Array();
foreach($friendsList['data'] as $i=>$friend){
	$friendID = $friend['id'];
	foreach($user_fb_uid as $i=>$blogUser){ 
		if($friendID == $blogUser){
			$bpFriends[] = $users[$i];
		}
	}
}
return $bpFriends;




So first they select EVERY member, tens of thousands of them and growing every day. Then they store the member ids(primary key) and the Facebook ids in two arrays. Then they iterate over the results of an Facebook API call that gets the logged in users friends and inside that loop they iterate through every one of the previously fetched members records(inexplicably naming the value variable $blogUser) seeing if they can find a match.... :omg:

I'm also seeing allot of things like this:
   $result = mysql_query("SELECT * FROM table WHERE field = somevalue");
   if($result)
   {
     $i = 0;
     while($data = mysql_fetch_object($result)
     {
         $i = $i + 1;
     }

    if($i > 0)
    .....
   }



There are thousands of lines of code like this. The code review has turned into an all day event when I was expecting it to take ~4 hours max. And this work did not come cheap, the company charged a fair price. I'm not even sure how to word the response I will eventually send with the code review....

Is This A Good Question/Topic? 0
  • +

Replies To: Awful PHP

#2 The_Programmer-  Icon User is offline

  • Death Scythe
  • member icon

Reputation: 24
  • View blog
  • Posts: 593
  • Joined: 24-October 11

Re: Awful PHP

Posted 21 October 2012 - 06:41 PM

Wow. I'm a beginner in PHP and I would never do this... I wonder how they got the job in the first place?
Was This Post Helpful? 1
  • +
  • -

#3 Jstall  Icon User is offline

  • Lurker
  • member icon

Reputation: 434
  • View blog
  • Posts: 1,042
  • Joined: 08-March 09

Re: Awful PHP

Posted 22 October 2012 - 08:48 AM

That is a very good question. I sent the code review in yesterday and now I've been asked to go through and fix the code as they do not trust the company that wrote it. I've agreed and will be working every evening this week to get things in order.
Was This Post Helpful? 1
  • +
  • -

#4 depricated  Icon User is offline

  • Behind Seven Proxies!

Reputation: 410
  • View blog
  • Posts: 1,421
  • Joined: 13-September 08

Re: Awful PHP

Posted 02 November 2012 - 07:56 AM

Wow...just wow.

I can just picture the storm when that code hits production.

Made me think of this
http://www.osnews.co...ry/19266/WTFs_m
Was This Post Helpful? 2
  • +
  • -

#5 JackOfAllTrades  Icon User is online

  • Saucy!
  • member icon

Reputation: 5950
  • View blog
  • Posts: 23,208
  • Joined: 23-August 08

Re: Awful PHP

Posted 02 November 2012 - 08:07 AM

Quote

the company that wrote it


Think I spotted your problem. Outsourcing -- without very strict oversight -- is a losing proposition.

I really need to start a business doing code reviews/cleanup for these messes. Just not sure how to advertise it or charge for it :/
Was This Post Helpful? 1
  • +
  • -

#6 Jstall  Icon User is offline

  • Lurker
  • member icon

Reputation: 434
  • View blog
  • Posts: 1,042
  • Joined: 08-March 09

Re: Awful PHP

Posted 03 November 2012 - 01:15 PM

Yes that is part of the problem, but the company was local and came recommended by some friend of the CEO's who worked with them before. That made it all the more surprising when I actually looked at what they had done.

Quote

I really need to start a business doing code reviews/cleanup for these messes.

Great idea! Although because the type of people who outsource generally do so because they don't want to pay for quality work, which would mean you would constantly be low-balled.
Was This Post Helpful? 2
  • +
  • -

#7 khedron  Icon User is offline

  • D.I.C Head

Reputation: 2
  • View blog
  • Posts: 52
  • Joined: 20-October 12

Re: Awful PHP

Posted 04 December 2012 - 12:52 PM

maybe that individual programmer did it so that they may exploit it later
Was This Post Helpful? 0
  • +
  • -

#8 jon.kiparsky  Icon User is online

  • Pancakes!
  • member icon


Reputation: 7289
  • View blog
  • Posts: 12,090
  • Joined: 19-March 11

Re: Awful PHP

Posted 04 December 2012 - 01:10 PM

View PostJstall, on 03 November 2012 - 03:15 PM, said:

Great idea! Although because the type of people who outsource generally do so because they don't want to pay for quality work, which would mean you would constantly be low-balled.


They may not want to pay for the work, but if they've used up their deadlines on the outsourcing, they may be in a mood to just pay someone to make it go away.
Was This Post Helpful? 0
  • +
  • -

#9 ishkabible  Icon User is offline

  • spelling expret
  • member icon





Reputation: 1616
  • View blog
  • Posts: 5,707
  • Joined: 03-August 09

Re: Awful PHP

Posted 07 December 2012 - 09:35 AM

I currently work for the Housing and Dinning department at my college. I do web application devolpment for them, I have to do similar things ALL THE TIME. Problem is all the code I maintain was written by other students who were only there for less than 4 months at a time. We haven't had a coordinator to review code or organize everything in 2 years. Basically imagine what you would get if you told a hand full of college students to get crakin' on about 20 different projects with no oversight; these people having no real world experience yet and piss poor programing skills. Because I haven't had the chance to create my own monstrosity for someone else to maintain I've been maintaining the code produced by this.

This post has been edited by ishkabible: 07 December 2012 - 09:49 AM

Was This Post Helpful? 0
  • +
  • -

#10 e_i_pi  Icon User is offline

  • = -1
  • member icon

Reputation: 782
  • View blog
  • Posts: 1,662
  • Joined: 30-January 09

Re: Awful PHP

Posted 12 December 2012 - 09:37 PM

Those variables declarations on lines 1 and 3 are gold.

Also, I love this part:
foreach($friendsList['data'] as $i=>$friend){
	$friendID = $friend['id'];
	foreach($user_fb_uid as $i=>$blogUser){ 
		if($friendID == $blogUser){
			$bpFriends[] = $users[$i];
		}
	}
}


"Well, we're not going to use the key part of $friendsList['data'], but let's just cast it as a key-val relationship anyway. While we're at it, let's clobber that key 2 lines later"

EDIT: Why are they declaring $friendID at all?!

This post has been edited by e_i_pi: 12 December 2012 - 09:39 PM

Was This Post Helpful? 0
  • +
  • -

#11 antolyevich  Icon User is offline

  • New D.I.C Head

Reputation: -6
  • View blog
  • Posts: 12
  • Joined: 02-September 12

Re: Awful PHP

Posted 14 December 2012 - 08:04 PM

This is how Facebook accounts get hijacked, just imagine if it got out....
Was This Post Helpful? 0
  • +
  • -

#12 creativecoding  Icon User is offline

  • Hash != Encryption
  • member icon


Reputation: 922
  • View blog
  • Posts: 3,195
  • Joined: 19-January 10

Re: Awful PHP

Posted 07 January 2013 - 04:20 PM

Awful code and PHP seem to always go hand in hand.

I'm going to assume this little thing resulted in the company losing money? I don't get outsourcing... I mean I understand paying less money for it, but don't you think the project managers would soon realize "...hey... my people are spending all of their time fixing this code we bought!" and cut it out? Eh.

Also this post made me realize something about contractors and programming... How can people prevent against unworthy code? If it works and completes the contract then i guess they did their job, but it's like asking someone to build a house and they use twigs for the support beams and leaves for the roofing. It may work now, but it won't later.

antolyevich:
I doubt third parties can hijack Facebook accounts using Facebook's own API :P Correct me if I'm wrong though.

This post has been edited by creativecoding: 07 January 2013 - 04:21 PM

Was This Post Helpful? 0
  • +
  • -

#13 Atli  Icon User is offline

  • D.I.C Lover
  • member icon

Reputation: 3635
  • View blog
  • Posts: 5,756
  • Joined: 08-June 10

Re: Awful PHP

Posted 07 January 2013 - 05:24 PM

View Postcreativecoding, on 07 January 2013 - 11:20 PM, said:

Awful code and PHP seem to always go hand in hand.

Not really, no. I've seen a lot of well written PHP code, as well as a lot of awful code written in other languages.

View Postcreativecoding, on 07 January 2013 - 11:20 PM, said:

How can people prevent against unworthy code? If it works and completes the contract then i guess they did their job, but it's like asking someone to build a house and they use twigs for the support beams and leaves for the roofing.

That depends on the contract. You'd no doubt have a plan ready for the house from an architect before you hire somebody to build it, right? You could apply some of that logic to software development contracts as well. If you outline a coding style and a some requirements that the code must satisfy (like: use a MVC framework, OOP for all code, and an ORM for the database), and put it in the contract that this must be adhered to when writing the code, you could sue their shoes of if at some point it is discovered that the code is not up to par. That would no doubt scare away some of the cheaper, less able outsourcing companies.

The problem would be finding an "architect" you trust to write those requirements, but it shouldn't be too hard.
Was This Post Helpful? 1
  • +
  • -

#14 creativecoding  Icon User is offline

  • Hash != Encryption
  • member icon


Reputation: 922
  • View blog
  • Posts: 3,195
  • Joined: 19-January 10

Re: Awful PHP

Posted 07 January 2013 - 05:46 PM

I didn't mean PHP always results in bad code, I meant to say that it's real easy create bad code in PHP and lots of "professionals" do so..

And that makes sense with the contract. Thanks!
Was This Post Helpful? 0
  • +
  • -

#15 xclite  Icon User is offline

  • LIKE A BOSS
  • member icon


Reputation: 877
  • View blog
  • Posts: 3,122
  • Joined: 12-May 09

Re: Awful PHP

Posted 07 January 2013 - 09:14 PM

View Postcreativecoding, on 07 January 2013 - 07:46 PM, said:

I didn't mean PHP always results in bad code, I meant to say that it's real easy create bad code in PHP and lots of "professionals" do so..

Damnit, that was the tired joke I was coming here to make.
Shucks.
/shuffle away
Was This Post Helpful? 0
  • +
  • -

Page 1 of 1