7
\$\begingroup\$

After initially struggling with the board size, I have now decided the dimension. Please review my code. In the code I am generating HTML code using PHP and MySql. I have not added the database, please let me know how to add it in my question.

The key functionality is drawBoard() it has 3 parameters,

  • 1 is to determine whether I will increment or decrease,
  • 2 determines where to start from
  • 3 determines where to end at

Please let me know if further clarification is required. I have also omitted the generated HTML instead here is a picture of what the output looks like. Let me know if generated HTML is needed?

I am looking for ways to improve the code and would like to know whether a more quicker loading time can be achieved. Any general suggestions are also welcome.

Index.php

<?php 
    include 'header.php';
    include 'functions.php';
?>
<div class="board">
    <div class="row" id="top-row">
        <?php drawBoard(1, 0, 11); ?>
    </div>
    <div class="row" id="center-row">
        <div class="left-column">
              <?php drawBoard(-1, 39, 30); ?>
        </div>
        <div class="center-colum">
        </div>
        <div class="right-column">
            <?php drawBoard(1, 11, 20); ?>
        </div>
    </div>
    <div class="row" id="bottom-row">
        <?php drawBoard(-1, 30, 19); ?>
    </div>
</div>
</body>
</html>

functions.php

<?php 

function drawBoard($diff = "", $start = "", $stop =""){

  include 'dbconnect.php';

  for ($i = $start; $i != $stop; $i += $diff){

    $criteria = "pos" . $i;
    $result = mysqli_query($con,"SELECT * FROM positions WHERE position = '$criteria'")
    or die("Could not select examples");

    while($row = mysqli_fetch_array($result)){

        $posid = $row[0];
        $title = $row[1];
        $set = $row[2];
        $withIcon = $row[3];

        if ($withIcon == 0) { ?>

          <div class="position">

                <div class="title">

                  <span class="<?php echo $set ?>"> <?php echo $title ?></span>

                </div>

                <div class="runway" id="<?php echo "runway" .$posid ?>">

                  <span class="piece-1" id="<?php echo "piece1"  .$posid ?>"></span>
                  <span class="piece-2" id="<?php echo "piece2" .$posid ?>"></span>

                </div>

                <div class="price">

                  <span class="price" id="<?php echo "price" .$posid ?>"></span> 

                </div>

          </div>
  <?php 
        }
        else { ?>

                <div class="position">

                  <div class="runway" id="<?php echo "runway" .$posid ?>">

                    <span class="piece-1" id="<?php echo "piece1"  .$posid ?>"></span>
                    <span class="piece-2" id="<?php echo "piece2" .$posid ?>"></span>

                  </div>

                  <div class="positionbody">

                    <span class="<?php echo $set ?>"></span> 

                  </div>

                </div>
<?php
        }; 
    }
  }
}  
?>

CSS

span{
    display: block;
    font: bold 10px "Arial", "Helvetica", sans-serif;
    color: #000;
    text-align: center;
}
/***********************
    Board Structure
************************/
.board {
    background: #EBEBE0;
    width: 845px;
}
.row{
    background: #fff;  
    float: left;
    width: 100%;
}
.left-column{
    width: 75px;
    float: left;
}
.center-column{
    float: left;
    width: 20px;
}
.right-column{
    float: right;
    width: 75px;
}
/************************
    Board Positions
************************/
.position {
    border: 1px solid #CFCFCF;
    float: left;
    height: 48px;
    margin: 0 2px 2px 0;
    width: 73px;
}
div .position:last-child {
    margin-right: 0;
}
/************************
    Positions Title
************************/
.title span{
    line-height: 14px;
}
.brown, .blue, .pink, .orange, .red, .yellow, .green, .navy{
    height: 14px;   
    color: white;
    width: 100%;
}
.brown{
    background: #AA6220;
}
.blue{
    background: #7AC0F6;
}
.pink{
    background: #FF356E;
}
.orange{
    background: #FF7A00;
}
.red{
    background: #C40101;
}
.yellow{
    background: #FFFF00;
    color: black;
}
.green{
    background: #0B860B;
}
.navy{
    background: #001791;
}

/************************
    Positions Runway
************************/
.runway{
    height: 12px;
    margin: 2px 0 2px 0;
    position: relative;
    z-index: 2;
}
.piece-1{
    background-image:url('../img/piece2.png');
    float: left;
    height:12px;
    margin-left: 2px;
    width: 12px;
}
/***********************
    Positions Body
************************/
.positionbody{
    height:32px;
    margin-top: -8px;
    position: relative;
    z-index: 1;
}
.go, .chest, .airport, .taxes, .chance, .jailhouse{
    background-repeat: no-repeat;
    width: 32px;
    height: 32px;
    margin: 0 auto 0 auto;
}
.go{
    background-image:url('../img/go.png');
}
.chest{
    background-image:url('../img/chest.png');
}
.taxes{
    background-image:url('../img/taxes.png');
}
.airport{
    background-image:url('../img/plane.ico');
    z-index: -1;
}
.airport-title{
    height: 12px;
    position: relative;
    top:-8px;
}
.chance{
    background-image:url('../img/chance.png');
}
.jailhouse{
    background-image:url('../img/jail.png');
}
/***********************
    Positions Price
************************/
.price{
    height: 14px;
    margin-top: 5px;
}

Post Edit

for ($i = $start; $i != $stop; $i += $diff){

This for loop gets its variables from the function parameters. I want to reduce my parameters to just 2 instead of 3. Parameter $diff tells the for loop to either i++ or i--. The only other way I can think of achieving this with just 2 parameters is using an if statement like if $start > $stop then $i-- but that would mean additional lines of code.

\$\endgroup\$
0

2 Answers 2

4
\$\begingroup\$

When mixing if statements and HTML I and others often use the alternative syntax, which makes it easier to spot what's being closed even though the brackets might have a great deal of distance between them:

if( condition ): ?>
    HTML
<?php endif;

I like the idea of using semantic variables instead of $row[index] but I think there are more elegant ways of making the assignments. For example by using mysqli_fetch_assoc you might get a semantic associative array that you could use and skip the extra assignments, or you could use

list($posid, $title, ...,...) = $row;

To fix the problem that you mention in your edit you could do for example:

for ($i = min($start,$stop); $i != max($start, $stop); $i += 1){

If it doesn't matter in which order you loop through the elements. In this case I guess it does, so you would have to get the sign of the difference ($stop-$start):

$diff = ($stop > $start) ? 1 : -1; 

Finally I think that the default values of drawBoard should be numeric.

\$\endgroup\$
3
\$\begingroup\$

DISCLAIMER: I don't know PHP. Also I haven't tested this.

Running on the assumption that position is a number or number-like value I figure you could do something like:

function drawBoard($order = "DESC", $start = "", $stop =""){

    include 'dbconnect.php';


     $result = mysqli_query($con,"SELECT * FROM positions WHERE REPLACE(position, 'pos', '') BETWEEN '$start' AND '$stop' ORDER BY position $order")
    or die("Could not select examples");


    while($row = mysqli_fetch_array($result)){

This is one call to SQL. Replace $diff with order *(holding either ASC or DESC). no for loop.

BUT the values in position would work better without the prefix "pos" and as a numeric value.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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