2010-04-11 2 views
1

나는 매우 깐깐한 코더로 평범하지 않은 것을 알아 차렸다.PHP 코드 정리하기

내 코드를 살펴보고 코드를보다 효율적으로 작성하는 방법에 대한 정보를 얻을 수 있습니까? 개선하기 위해 무엇을 할 수 있습니까?

session_start(); 



/*check if the token is correct*/ 
if ($_SESSION['token'] == $_GET['custom1']){ 

    /*connect to db*/ 
    mysql_connect('localhost','x','x') or die(mysql_error()); 
    mysql_select_db('x'); 



    /*get data*/ 

    $orderid = mysql_real_escape_string($_GET['order_id']); 
    $amount = mysql_real_escape_string($_GET['amount']); 
    $product = mysql_real_escape_string($_GET['product1Name']); 
    $cc = mysql_real_escape_string($_GET['Credit_Card_Number']); 
    $length = strlen($cc); 
    $last = 4; 
    $start = $length - $last; 
    $last4 = substr($cc, $start, $last); 
    $ipaddress = mysql_real_escape_string($_GET['ipAddress']); 
    $accountid = $_SESSION['user_id']; 
    $credits = mysql_real_escape_string($_GET['custom3']); 




    /*insert history into db*/ 
    mysql_query("INSERT into billinghistory (orderid, price, description, credits, last4, orderip, accountid) VALUES ('$orderid', '$amount', '$product', '$credits', '$last4', '$ipaddress', '$accountid')"); 
    /*add the credits to the users account*/ 
    mysql_query("UPDATE accounts SET credits = credits + $credits WHERE user_id = '$accountid'"); 

    /*redirect is successful*/ 
    header("location: index.php?x=1"); 
}else{ 

    /*something messed up*/ 
    header("location: error.php"); 
} 

답변

3

프레임 워크 또는 적어도 객체 관계형 매퍼를 사용하는 것이 좋습니다.

당신이 객체 지향 접근법을 사용한다면 그것은 SQL 연산을 매우 쉽게 만듭니다.

예를 들어 내 프레임 워크에서는 등록 양식을 작성한 후 새 사용자를 만드는 데 몇 줄의 코드 만 필요합니다. 물론 재미있는 비즈니스 나 해킹 시도를 막기 위해 객체 뒤에 캡슐화 된 수많은 코드가 있지만 작업하기가 매우 쉽습니다.

$account = new Account(); 
$account->insert(array(
    'userid' => $id, 
    'accountpass' => $passhash, 
    'accountemail' => $emailhash 
)); 

나는 어떤 잘 알려진 프레임 워크를 사용하거나 관계 매퍼 객체,하지만 난 CodeIgniter를, Kohana, 교리는 몇 가지 좋은 것들 들었어요하지 않았습니다. 그러나 그들은 학습 곡선을 가지고 있으므로 응용 프로그램이 다른 API를 학습 할만큼 충분히 큰지 미리 알아야합니다.

+0

전에 Codeigniter를 사용했지만 실제로 사용하지 않았습니다. –

+0

그런데 어느 프레임 워크를 사용하고 있습니까? –

+0

OR 매핑을 위해 PDO :: fetchObject()를 찾으십시오. – Danten

4

코드가 꽤 표준적인 것처럼 보입니다. 아마도 일종의 들여 쓰기를 사용해야하며, MySQL 쿼리의 경우에는 쿼리 중간에 변수를 포함하는 대신 prepared statements을 사용하십시오. 스타일의 관점에서 볼 때 대부분 괜찮습니다.

+0

고마워요, 이걸 살펴볼 것입니다! –

1

"깨끗한"의미에 따라 다릅니다. 모든 것이 기본 들여 쓰기에서 대단히 유익 할 것입니다. 기존의 공백을 더 생산적인 곳으로 옮깁니다. 들여 쓰기와 줄 바꿈 일관성은 코드 가독성을 크게 높입니다.

다음 수준의 스타일에서 원시 MySQL 함수로 작업 할 때 왜 거기에 많은 변수를 할당해야하는지 알 수 있습니다. 해당 시스템을 고수 할 계획이라면 적어도 변수를 합리적인 순서로 지정해야합니다. 아래에서 몇 가지 다른 변경 사항과 메모를 작성해야합니다.

// Let's just strip all non-numerics from the card number so that we can validate it. 
$cc = preg_replace('/[^0-9]/', '', $_GET['Credit_Card_Number']); 
if(strlen($cc) != 12) { 
    // The card number is invalid; go back to form and try again. 
} 
$last4 = substr($cc, -4); // A negative start value is cleaner. 

$accountid = $_SESSION['user_id']; 

$amount = mysql_real_escape_string($_GET['amount']); 
$credits = mysql_real_escape_string($_GET['custom3']); 
// note: don't trust that users won't change the ipAddress parameter before submitting 
$ipaddress = mysql_real_escape_string($_GET['ipAddress']); 
$orderid = (int) $_GET['order_id']; 
$product = mysql_real_escape_string($_GET['product1Name']); 

변수 할당을 세 블록으로 나누었습니다. 첫 번째는 데이터 변환을위한 것이고 두 번째는 GET이 아닌 소스에서 데이터를 가져 오는 것입니다. 세 번째는 알파벳 순으로 정렬 된 $_GET 변수의 기본 할당입니다. 과제는 이제 훨씬 더 구조화되어 있으며 주어진 라인을 쉽게 찾을 수 있습니다. (물론 이것은 어떤 방법으로도 문제를 해결할 수있는 표준 방법이 아니며 여기서 의미가있는 것입니다.)

좋은 코드에 관한 모든 것이 중요하므로 유지 관리가 쉽습니다. 무엇이 가장 잘 작동하는지는 모르겠지만, 장기적으로 코드 가독성과 체계적인 변수 할당이 잘 작동한다고 생각합니다.

다른 언급했듯이 데이터베이스 작업에는 PDO과 같은 것을 사용하십시오. 이렇게하면 준비된 명령문을 사용하고 모든 이스케이프 기능을 피할 수 있습니다. Doctrine 또는 본격적인 framework과 같은 ORM을 살펴볼 수도 있습니다. 더 강력한 도구를 사용하면 학습 곡선이 향상되지만 OOP에 능숙 해지면 생산성이 향상됩니다.

+0

최종 사용자가 하이픈을 입력하거나'(int) '공백을 입력하면'$ _GET ['Credit_Card_Number ']'를 캐스팅하는 것이 너무 잘 작동하지 않습니다. 4016 5432 1234 "= 4016 ' – deadkarma

+0

흠. 나는 편집을 할 것이지만 OP의 전체 유효성 검증을 쓰지 않고도 편집 할 수 있을지는 모르겠다. 왜냐하면 나는 신용 카드 번호를 문자열처럼 저장하는 것을 바보처럼 삼고 싶지 않기 때문이다. . 분명히 모든 숫자가 아닌 문자를 제거하고 정수로 저장해야합니다. 어쨌든, 그 효과에 대한 코드를 작성해 보겠습니다. – Matchu

+0

@deadkarma : 기본 유효성 검사가 추가되었습니다. Luhn 알고리즘에 신경 쓰지는 않습니다. 왜냐하면 실제로 지불을 처리하는 데 사용하는 것이 무엇이든지간에 그렇게 생각하기 때문입니다. – Matchu

3

GET을 통해 신용 카드 번호와 IP 주소를 전달하는 것은 안전하지 않으므로 GET 문자열이 포함 된 URL이 브라우저 기록에 남을 수 있습니다. $_GET['ipAddress']은 어떤 방식으로 지정 하시겠습니까?대신 $_SERVER['REMOTE_ADDR']을 사용해야합니다.

신용 카드의 마지막 4 자리 숫자를 얻으려면, 당신은 수행하여 몇 줄의 코드를 저장할 수 있습니다

가 꽤 큰이

$last4 = substr($cc,-4);

+0

그건 내 선택이 아니야. 내 CRM은 주문의 모든 정보를 GET을 통해 내 성공 페이지 (IP 주소 포함)로 전달합니다. –

1

나를 밖으로 도약 가장 먼저하는 일이된다 코드의 보안 구멍. $ _GET 변수를 SQL 쿼리에 직접 입력하면 광범위한 공격에 노출 될 수 있습니다. 또한 청구서 수신에 데이터를 사용하는 것처럼 보일 때 get 변수를 사용하여 자신의 계정 잔액을 조작 할 수 있습니다.

들어오는 GET 데이터를 데이터베이스로 덤핑하기 전에 일부 데이터 검사를 권장합니다.

+0

아, 전화하세요. 각 패키지가 얼마나 많은 크레딧을 가지는지 확인할 수 있습니다. \t if ($ product == "Bronze") { \t $ credits = 400; \t} elseif ($ product == "Silver") { \t $ 크레딧 = 1042; \t} elseif ($ product == "Gold") { \t $ 크레딧 = 2275; \t} elseif ($ product == "Platinum") \t $ 크레딧 = 5000; \t} 하지만 여전히 패키지 이름을 변경할 수 있습니다. 이 일을 어떻게 권장 하시겠습니까? –

+1

크레딧 정보를 보내는 것을 피할 수 있습니까? 제품 별 크레딧을 어딘가에 데이터베이스에 저장하고 해당 데이터를 사용할 수 있습니다. 또한 제품에 문자와 숫자 만 포함되어 있는지 확인하십시오 (이렇게하면 많은 공격을 막을 수 있으며 대부분 쉼표 등이 포함되어 있음) – paullb

1

동일한 동작을 반복 할 때마다 기능을 추가하는 것이 좋습니다.

예를 들어 데이터를 이스케이프하는 방식을 변경하려면 예를 들어 mysql_real_escape_string을 사용하는 모든 행을 편집해야합니다.

당신이 배열 (여기 $ _GET)의 모든 구성원에게 동일한 기능을 적용하는 경우, 같은

function escape_data($data) { 
    $escaped_data = mysql_real_escape_string($data); 
    // room for more actions ... 
    return ($escaped_data); 
} 

또한 기능을 추가하지 않는 이유

, 당신은 array_map() 유용하게 찾을 수 있습니다 :

$_GET = array_map('escape_data', $_GET);