up vote 1 down vote favorite

I am fairly new to PHP, and would love to have my code reviewed to see what I am doing wrong and ways to improve it. The code works fine, I just feel like there's an easier way to do this.

This is the PHP code at the top of my page - and then the HTML is below it, but I will only paste the PHP code. What the page is, is a way for a shop to edit which credit cards they accept.

Instead of a form, I have the credit card logos on the page and if it's selected in the database then it is highlighted at 100% opacity, and if it's not selected then it's at 25% opacity, so it's faded out. If they choose to start accepting Mastercard for example, they click on the faded Mastercard logo and it goes to ?card=mastercard&value=1 and it updates the database and refreshes the page, so when it refreshes it is now full opacity and selected in the database.

In the database it is called "credit_cards" and the values are "xxxx" for "visa, mastercard, discover, amex"

So if they are all selected, it will show "1111" if all except discover then it is "1101"

Here is the code:

<?php
require_once('MySqlDb.php');
$Db = new MySqliDb('localhost', 'root', 'root', 'shops');

if(!isset($_GET['user_id'])) {
die("No user selected.");
}
$id = $_GET['user_id'];

$Db->where('id', $id);
$result = $Db->get('users');

if(isset($_GET['card']) && isset($_GET['value'])) {

if ($_GET['value'] == "1" || $_GET['value'] == "0") {

$card = $_GET['card'];
$value = $_GET['value'];

// Get Current Credit Cards Value In Database
$currentCreditCards = $result[0]['credit_cards'];

// Individual Credit Card Values
$visa = substr($currentCreditCards, 0, 1);
$mastercard = substr($currentCreditCards, 1, 1);
$discover = substr($currentCreditCards, 2, 1);
$amex = substr($currentCreditCards, 3, 1);

switch($card) {
    case "visa":
        // Get The New Value To Update The Database With
        $newCreditCards = $value ."". $mastercard ."". $discover ."". $amex;

        $updateData = array('credit_cards' => $newCreditCards);

        $Db->where('id', $id);
        $Db->update('users', $updateData);
        header("location: test.php?user_id=$id");
    break;

    case "mastercard":
        $newCreditCards = $visa ."". $value ."". $discover ."". $amex;

        $updateData = array('credit_cards' => $newCreditCards);

        $Db->where('id', $id);
        $Db->update('users', $updateData);
        header("location: test.php?user_id=$id");
    break;

    case "discover":
        $newCreditCards = $visa ."". $mastercard ."". $value ."". $amex;

        $updateData = array('credit_cards' => $newCreditCards);

        $Db->where('id', $id);
        $Db->update('users', $updateData);
        header("location: test.php?user_id=$id");
    break;

    case "amex":
        $newCreditCards = $visa ."". $mastercard ."". $discover ."". $value;

        $updateData = array('credit_cards' => $newCreditCards);

        $Db->where('id', $id);
        $Db->update('users', $updateData);
        header("location: test.php?user_id=$id");
    break;
}

}
}

foreach($result as $row) :

if($row['credit_cards'][0] == "1"){ 
    $visaNumber = "0";
    $visaStyle = "style=\"opacity: 1; filter: alpha(opacity=100)\""; 
} else { 
    $visaNumber = "1";
    $visaStyle = "style=\"opacity: 0.25; filter: alpha(opacity=25)\"";
}
if($row['credit_cards'][1] == "1"){ 
    $masterCardNumber = "0";
    $masterCardStyle = "style=\"opacity: 1; filter: alpha(opacity=100)\""; 
} else { 
    $masterCardNumber = "1";
    $masterCardStyle = "style=\"opacity: 0.25; filter: alpha(opacity=25)\"";
}
if($row['credit_cards'][2] == "1"){ 
    $discoverNumber = "0";
    $discoverStyle = "style=\"opacity: 1; filter: alpha(opacity=100)\""; 
} else { 
    $discoverNumber = "1";
    $discoverStyle = "style=\"opacity: 0.25; filter: alpha(opacity=25)\"";
}
if($row['credit_cards'][3] == "1"){ 
    $amexNumber = "0";
    $amexStyle = "style=\"opacity: 1; filter: alpha(opacity=100)\""; 
} else { 
    $amexNumber = "1";
    $amexStyle = "style=\"opacity: 0.25; filter: alpha(opacity=25)\"";
}
?>

Is this a stupid way of doing it? Am I doing this wrong?

The whole visaNumber and visaStyle stuff is just for the HTML, so if its selected in the database, then the URL needs to point to ?visa=0 to turn it off, instead of ?visa=1, and then it has to show the correct opacity.

Thanks so much for any help.

share|improve this question

2 Answers

up vote 2 down vote accepted

The first part that updates the selected credit card has a lot of code duplication that could be made generic.

// Get Current Credit Cards Value In Database
$userCards= $result[0]['credit_cards'];

$CARDS = array('visa', 'mastercard', 'discover', 'amex');
$cardIndex = array_search($card, $CARDS);
if ($cardIndex !== false) {
    $userCards[$cardIndex] = $value;
    $updateData = array('credit_cards' => $userCards);
    $Db->where('id', $id);
    $Db->update('users', $updateData);
    header("location: test.php?user_id=$id");
}

Update: Start by replacing named variables such as $visaNumber and $visaStyle with an array of number/styles so these become $cardNumber[0] and $cardStyle[0]. Add another loop to assign the numbers and styles.

$cardNumbers = array();
$cardStyles = array();
foreach ($result as $row) {
    foreach ($CARDS as $cardIndex => $cardName) {
        if ($row['credit_cards'][$cardIndex] == "1") {
            $cardNumbers[$cardIndex] = "0";
            $cardStyles[$cardIndex] = 'style="opacity: 1; filter: alpha(opacity=100)"';
        } else {
            $cardNumbers[$cardIndex] = "1";
            $cardStyles[$cardIndex] = 'style="opacity: 0.25; filter: alpha(opacity=25)"';
        }
    }
}

Without seeing how the numbers and styles are used, I may have gotten the above wrong. What exactly does $result hold? What is in each $row?

share|improve this answer
Wow! That really did shorten the code up tremendously while still doing the same thing. Exactly what I was looking for, I knew I was approaching it the wrong way. Thank you so much. Do you have a method of shortening the bottom part with the cardNumber and cardStyle? – Drew Jun 20 '11 at 2:57
@Drew - See my update. – David Harkness Jun 20 '11 at 5:34
Thank you! Now I am starting to understand. Yeah, $result just holds what is in each row. – Drew Jun 20 '11 at 12:29
up vote 0 down vote

Why do you store credit cards data in the database ? What is the purpose of this information?

You should use CSS classes in your code:

.deselected {
  opacity: 0.25; filter: alpha(opacity=25)
}

The selected cards need no additional style, HTML elements are already fully opaque.

share|improve this answer
Thank you! That is true, that will help shorten up that part of the code a lot. The page that I posted is a settings page that clients can log in and see - so they choose themselves which credit cards they want to appear on their website. Their website is dynamically created, so it searches the database to get which cards that specific user has selected and it shows the appropriate ones. Does that make sense? Is there a better way of doing it? – Drew Jun 20 '11 at 18:39

Your Answer

 
or
required, but never shown
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.