I help kids in my grade with economics class, so I am working on a little shopping things. I made a ‘bank’ system where they can store ‘money’ and make purchases. Before I do anything and possibly ruin my database, can you please check my scripts for errors / bugs / etc.
<?php
$error = "0"; //error holder
//short-term array holders
$cashs = array();
$fnames = array();
$lnames = array();
$saps = array();
//define user login details
$si = $_GET["s"]; //Sellers ID
$ui = $_GET["u"]; //(USERS) Buyers ID
$co = $_GET["c"]; //Cost
//define database login details
$servername = "localhost";
$username = "use";
$password = "";
$dbname = "bank";
//pre-transaction functions
function userget ($whom) {
//get database content of user.
// Create connection / load total array
$conn = new mysqli($servername, $username, $password, $dbname);
// Check connection
if ($conn->connect_error) {
die("Connection failed: " . $conn->connect_error);
$error = "0200";
}
$sql = "SELECT cash, fname, lname, sap FROM bank1";
$result = $conn->query($sql);
$x = 0;
if ($result->num_rows > 0) {
// output data of each row
while($row = $result->fetch_assoc()) {
$cashs[$x] = $row["cash"];
$fnames[$x] = $row["fname"];
$lnames[$x] = $row["lname"];
$saps[$x] = $row["sap"];
$x++;
}
} else {
echo "0 results <br>";
$error = "0201";
}
$conn->close();
//define your info
$x = 0;
$b = count($saps) - 1;
while ($x <= $b) {
if ($whom == $saps[$x]) {
$name["f"] = $fnames[$x];
$name["l"] = $lnames[$x];
$cash = $cashs[$x];
}
$x++;
}
}
function pre1 () {
//check if the user can afford the item.
if ($cash < 0) {
$error = "0100";
}
}
function taw() {
$conn = new mysqli($servername, $username, $password, $dbname);
// Check connection
if ($conn->connect_error) {
die("Connection failed: " . $conn->connect_error);
}
$sql = "UPDATE bank1 SET cash='$rem' WHERE sap=$ui";
if ($conn->query($sql) === TRUE) {
echo "Record updated successfully!";
} else {
echo "Error updating record: " . $conn->error;
}
}
function gto {
$conn = new mysqli($servername, $username, $password, $dbname);
// Check connection
if ($conn->connect_error) {
die("Connection failed: " . $conn->connect_error);
}
$sql = "UPDATE bank1 SET cash='$nem' WHERE sap=$si";
if ($conn->query($sql) === TRUE) {
echo "Record updated successfully!";
} else {
echo "Error updating record: " . $conn->error;
}
}
//start
userget($ui);
$rem = $cash - $co;
pre1();
userget($si);
$nem = $cash + $co;
$error = "1100":
?>
Although you can get one or two of them fixed here, it won’t help with global design flaws and insufficient level of expertize.
So, if you are really concerned in not ruining your database, the mission is rather impossible, I’d say. Yet, if this is a toy project you intend to learn from, then, after getting this bank hacked by them kids several times, you will get some experience.
You’re a sitting duck with that code for SQL Injection attacks, as you’re not even escaping user submitted data. When dealing with a query that gets something from an external source that it’s going to use eg something in the WHERE clause you must ALWAYS use prepared statements, no matter what the source of the variable that you’re going to use.
Also any user submitted data ALWAYS needs to be validated, so you don’t end up sending a load of junk to the database. It doesn’t matter how well you trust the user, any user submitted data should** ALWAYS** be considered unsafe and/or garbage until it has been VALIDATED
I’m not seeing where the two functions that have WHERE queries are ever being called.
For a small school project for demonstration purposes only I guess an IN would work OK.
What I would start with is simplifying the userget function.
Instead of getting all the rows, putting them into an array, then picking only one from the array, I think it would be more efficient to only get the one row you’re interested in.
And yes, even if this is only for a demonstration, validating user input is a very important thing to do and a habit you should acquire now.
I think you need to read up on variable scope, unless there’s something I’m missing. You seem to create a lot of variables inside functions, then access them outside of those functions, and vice versa.
On the one hand, it’s fair to say that teaching people PHP and SQL you should start with input validation, prepared statements and really stress the security implications of everything, but you also say that you’re teaching them economics rather than coding, so as long as the end results are correct and you don’t show them the code, it shouldn’t matter.
But there are strange things about the code. For example you get the user details for the buyer, create a variable called $rem which is the remaining cash (assuming there is a value in $cash as you don’t return it from the userget() function), then call the pre1() function, which then checks the value in $cash, not the value in $rem.
Have you run the code? Does it do what you want it to? I can’t see that it would, but it also might be part of a larger set of code.
Thank you. I know of variable scope, but didn’t realize I did it reverse. oops. No, I have not yet run the code. I did the functions like that for this reason: 1) I had to find how much cash the person buying has 2) Take away cash 3) Check how much money the buyer has 4) Add the cash to their account.
I do it like this because as far as I am aware, there is no direct way to say to the database (Add 10 dollars) without doing a math statement ($current + $cost = $new; update cash=‘$new’) Yes I realize that is not correct PHP in my example.
That you for your ‘constructive’ criticism. I posted it on this site for the reason of advice on insuring the web service works as intended. “Thank you” is what I would say to somebody who provided helpful advice.
Like I said before, no “helpful criticism” can salvage this code, of which every single line have to be rewritten.
You need to understand that no review can make it sure that a service works as intended. People aren’t computers. They aren’t intended to run a code in their heads. They can spot an error or two, but to make sure you need to run your code using a computer, and perform some tests.
Besides, asking to review a code you didn’t even bother to run is impolite at least.
So you better stick to asking concrete questions regarding particular problems you face to gradually improve your knowledge. Say, were you asked a question
is there a direct way to say to the database (Add 10 dollars)?
You’d be given an extremely constructive and helpful answer in an instant: yes, that’s what a database exactly for.
As long as it is your database, don’t worry about breaking it.
If it fills up with bad data just run a
TRUNCATE [TABLE] tbl_name`
for each bad table and start over http://dev.mysql.com/doc/refman/5.7/en/truncate-table.html
Or you may find you need to do ALTERs or even DROP and do CREATEs again.
But you will not “break” your database.
I think part of the reason some are coming across a bit harsh is because the title says “reviewing”.
This (on this forum anyway) generally means “I think this code is about ready, what do you think”
Apparently not everyone recognized the code as being that of someone relatively new to coding or have forgotten how their first attempts looked.
True, there is much that could be done to improve it, and maybe scrapping it all and starting anew would be best.
Can you “spec out” what you want the code to do in a outline type format?
If after that you [quote=“colshrapnel, post:11, topic:218928”]
stick to asking concrete questions regarding particular problems you face
[/quote]
you should indeed have better luck getting help.
Well, there’s something like this, though I’m not entirely sure my syntax is spot on
UPDATE bank1 SET cash=cash-7.99 WHERE sap=100
So you can use direct SQL to update your database figures. And in fact that’s a useful thing to consider as soon as you think that more than one user might be accessing your database, one might even suggest it’s mandatory unless you employ some kind of manual locking mechanism.
Think of the way your code works:
read the customer details into local variables
subtract the payment from the balance
write the new balance back to the customer details
Now consider what happens if there’s another user on your site, a fraction of a second behind, processing another transaction of a different value for the same customer. Let’s say that user gets the customer details while your first user is at position 2 in your code, so they get the original balance. Your first user now writes their updated balance back to the customer record, and the second user subtracts their transaction amount from the original balance and writes that back to the customer. You’ve done two operations on the same customer, but the balance is now incorrect.
User1 reads balance from table ($100)
User2 reads balance from table ($100)
User1 subtracts payment of $10, internal copy of balance now $90
User2 subtracts payment of $25, internal copy of balance now $75
User1 stores new balance of $90 to table
User2 stores new balance of $75 to table
Payments of $35 processed, balance incorrect as it should be $65.