Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I have some troubles to find the right loop to check if some values are contained in mysql DB. I'm making a software and I want to add license ID. Each user has x keys to use.

Now when the user start the client, it invokes a PHP page that check if the Key sent in the POST method is stored in DB or not.

If that key isn't store than I need to check the number of his keys. If it's > than X I'll ban him otherwise i add the new keys in the DB.

I'm new with PHP and MYSQL. I wrote this code and I would know if I can improve it.

<?php

$user = POST METHOD 
$licenseID = POST METHOD

$resultLic= mysql_query("SELECT  id , idUser , idLicense FROM license WHERE idUser = '$user'") or die(mysql_error());
$resultNumber = mysql_num_rows($resultLic);
$keyFound = '0'; // If keyfound is 1 the key is stored in DB

while ($rows = mysql_fetch_array($resultLic,MYSQL_BOTH)) {
   //this loop check if the $licenseID is stored in DB or not
    for($i=0; $i< $resultNumber ; i++) { 
        if($rows['idLicense'] === $licenseID) {
            //Just for the debug 
            echo("License Found");
            $keyFound = '1';
            break;
    }
    //If key isn't in DB and there are less than 3 keys the new key will be store in DB

    if($keyfound == '0' && $resultNumber < 3) { 
        mysql_query( Update users set ...Store $licenseID in Table) 
    }

    // Else mean that the user want user another generated key (from the client) in the DB and i will be ban (It's wrote in TOS terms that they cant use the software on more than 3 different station)                    
    else { 
        mysql_query( update users set ban ='1'.....etc );
    }                               
}

?>          

I know that this code seems really bad so i would know how i can improve it. Someone Could give me any advice?

I choose to have 2 tables: users where all information about the users is, with fields id, username, password and another table license with fields id, idUsername, idLicense (the last one store license that the software generate)

share|improve this question
1  
What does "improve it" mean? Does your code actually work? Or is it broken and you need help fixing it? This site is not for specific problems with code, but we can migrate it to the right place for you: Code Review if the code works, Stack Overflow if it doesn't. Please respond here using the "add comment" link and let me know. Thanks. – Anna Lear Nov 15 '11 at 17:24
@AnnaLear How fast do you type? :) – Yannis Nov 15 '11 at 17:27
@YannisRizos Pretty fast. :) – Anna Lear Nov 15 '11 at 17:27
Sorry for that , i just posted on StackOverflow and a comment says to post Here! I'm sorry about it – Jasper Nov 15 '11 at 18:14
@Jasper Ah, from the comments on your Stack Overflow post, I gather that your code is working? I'll migrate your question to Code Review and see if they can help you out. – Anna Lear Nov 15 '11 at 20:45

migrated from programmers.stackexchange.com Nov 15 '11 at 20:45

2 Answers

Don't use mysql_* functions. They have long been abandoned in favour of mysqli, which stands for MySQL Improved. mysql_* are targeting MySQL up to version 4.1.3, which is pretty old. The mysqli_* functions are exactly the same, just add that i and your code will work as before. You should also take a look at PDO which defines a lightweight, consistent interface for accessing multiple different databases.

Now, if by this:

$licenseID = POST METHOD

you mean this:

$licenseID = $_POST["licenceID"];

then you should really validate and sanitize $licenceID. Easiest way to do it is via the filter functions. And here:

($rows['idLicense'] === $licenseID)

you are using the equal and of the same type operator (===), if that's intentional that's perfectly fine, but if you just want to check for equality you should use == instead. If $licenceID is integer then bear in mind that:

1 === "1" // false
2 == "2"  // true

More on comparisons, in the manual. Other than that, your code is pretty typical and looks fine.

share|improve this answer

First of all: both code and table design can (have?) to be improved:

  1. Tables must be normalized and can be improved by removing data and duplicates
    • Users table can (?) have username only as natural PK (pure id eliminated)
    • Optional: licence table will be better understandable if FK will have same name as for Users table (username)
  2. For-cycle for your task is not-so-good style of coding:
    • You can just test result (size of array) of SELECT idLicense WHERE idLicense = $licenseID (SQLGuru: Fixme!!!)
    • Total amount of licences per user can be extracted from table with single SELECT COUNT(idLicense) WHERE idUser = '$username' (SQLGuru: Fixme again!!!)
share|improve this answer

Your Answer

 
discard

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