ddmbest88 Posted December 15, 2015 Share Posted December 15, 2015 (edited) Hello guys and thank in advance for help. I have a custom game server and i am building maps that shows wheter a fortress has been conquered and by who and show the correct faction icon upon the map. here is my script: <?php include_once 'dbconnectgs.php'; //creating a connection with the database ?> <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> <html xmlns="http://www.w3.org/1999/xhtml"> <head> <link rel="stylesheet" href="styles/eye.css"> </head> <body> <?php $Divine=mysql_query("SELECT id,race FROM siege_locations WHERE id = '1011'");// selecting the race of the conquering faction from the table of the fortresses where the id of the fortress is 1011 ?> <div id ="Divine"> <?php $row = mysql_fetch_row($Divine); //fetching data //echo $row[1] showing resoult just to be sure that data are ok: it shows correct resoult BALAUR if ($row = "ELYOS") { //conditions to show the icon, i have 3 options: faction 1 faction 2 or computer faction 3 echo "<img src=\"http://someaddress.me/sieges/img/fortezze/elyos.png\" border=0>"; } elseif ($row = "ASMODIANS") { echo "<img src=\"http://someaddress.me/sieges/img/fortezze/asmodian.png\" border=0>"; } elseif ($row = "BALAUR") { echo "<img src=\"http://someaddress.me/sieges/img/fortezze/balaur.png\" border=0>"; } ?> </div> </body> </html> Now problem is that even if the fetch shows the fortress as currently conquered by faction 3 (balaur) the if condition thinks that it's always faction 1 (elyos), and shows faction 1 logo, where do i mess up? Edited December 15, 2015 by ddmbest88 Quote Link to comment Share on other sites More sharing options...
mac_gyver Posted December 15, 2015 Share Posted December 15, 2015 (edited) one = is an assignment operator. two == is a comparison operator. the conditional tests in your code are assigning values to $row, then testing the value that was just assigned. your code is not testing the value in $row. also, $row is an array, from the msyql_fetch_row() statement (the mysql_ functions are obsolete and have been removed from the version of php that was just released, you should be using PDO or msyqli_ statements.) it will never be equal to those string values. you would need to test a specific element in the array in $row. in general, you should use an associative fetch statement, so that your code is more readable, and more maintainable. using $row['race'], instead of $row[1], will make it easier to write and read your code, both for you and for anyone else that must deal with it. lastly, DRY (Don't Repeat Yourself.) your code should not repeat program logic and html markup over and over, when just a small part of it is the dynamic/changing part. ideally your mapping of race values to image base names should be one to one, with the same image base name as the race value. this would eliminate all the if/elseif logic. you would have just one line of code - echo "<img src=\"http://someaddress.me/sieges/img/fortezze/{$row['race']}.png\" border=0>"; if for some reason you cannot make the race value and the image base name the same, you should use a simple mapping array to give the image base name that corresponds to the race name - $map["ELYOS"] = 'elyos'; $map["ASMODIANS"] = 'asmodian'; $map["BALAUR"] = 'balaur'; // add any other race/image base name pairs here... echo "<img src=\"http://someaddress.me/sieges/img/fortezze/{$map[$row['race']]}.png\" border=0>"; Edited December 15, 2015 by mac_gyver Quote Link to comment Share on other sites More sharing options...
ddmbest88 Posted December 15, 2015 Author Share Posted December 15, 2015 can i ask to correct my code and explain how you woyld do it just to understand for future things? Quote Link to comment Share on other sites More sharing options...
cyberRobot Posted December 15, 2015 Share Posted December 15, 2015 Basically, you would change lines like this if ($row = "ELYOS") { To this if ($row[1] == "ELYOS") { For what it's worth, you could use mysql_fetch_assoc() to make the code a bit more readable. More information about the function, including examples, can be found here: http://php.net/manual/en/function.mysql-fetch-assoc.php Quote Link to comment Share on other sites More sharing options...
Psycho Posted December 15, 2015 Share Posted December 15, 2015 Some tips: 1. Put all your logic (i.e. PHP code) at the top of the script. Use it to assign dynamic content to variables. Then create the output (i.e. HTML) at the bottom of the script using just PHP echo statements for the content created within PHP. This separates the logic from the presentation. As you build more complicated applications you will completely separate the logic and presentation into separate files. but, for now, go ahead and use a single script. 2. mysql_fetch_row() will return an array into $row, but then you are testing the value against a string. They will never compare. Plus, you are returning id and race, but only looking for one value apparently. Just pull the data you need. 3. Instead of multiple if/elseif statements, use a switch() statement 4. There is no else condition. Perhaps there are no records where a value would not equal one of those options, but it is a bad design. Code should never 'assume'. Create conditions to handle unintended scenarios. 5. The three conditions in your if/elseif statements should only define what is different - in this case just the src for the image tag. Right now, it is fairly simple, but since each condition will output an image, that part should not be included in the condition statements. As the output of such conditions gets more complicated it becomes all too easy to introduce bugs. For example, if you want to change the border of the image to 1 you would have to change it in three places instead of just one. 6. Use PDO or mysqli for database operations. mysql_ has been deprecated for years. 7. If the select statement will take a user defined parameter for the id, you should be using prepared statements Not tested <?php //Set the race ID to be queried //Can be user supplied via $_GET/$_POST $raceID = '1101'; //creating a connection with the database include_once 'dbconnectgs.php'; //Replace the contents of 'dbconnectgs.php' with the following // - using appropriate values for host, database bname, username & password ## $db = new PDO('mysql:host=localhost;dbname=testdb;charset=utf8', 'username', 'password'); ## $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); ## $db->setAttribute(PDO::ATTR_EMULATE_PREPARES, false); //Create & run prepared statement $query = "SELECT race FROM siege_locations WHERE id=:id LIMIT 1"; $stmt = $db->prepare($query); $stmt->execute(array(':id' => $raceID)); //Get the race from the query results $row = $stmt->fetchAll(PDO::FETCH_ASSOC); $race = $row['race']; //Determine correct image name switch($race) { case 'ELYOS': $imgName = "elyos.png"; break; case 'ASMODIANS': $imgName = "asmodian.png"; break; case 'BALAUR': $imgName = "balaur.png"; break; default: $imgName = "unknown.png"; } //Define the full IMG tag one with just the variable image name $raceImage = "<img src=\"http://someaddress.me/sieges/img/fortezze/{$imgName}\" border=0>"; ?> <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> <html xmlns="http://www.w3.org/1999/xhtml"> <head> <link rel="stylesheet" href="styles/eye.css"> </head> <body> <div id ="Divine"> <?php echo $raceImage; ?> </div> </body> </html> Quote Link to comment Share on other sites More sharing options...
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.