2
\$\begingroup\$

When we select the 'info' list in the select field, the article with type 'info' characteristic automatically showed up, and when we select 'Berita' list in the select field, the articles with type 'Berita' characteristic automatically show up.

This is a long script that uses a post method. Can you help me make it simpler, perhaps using functions?

<form method="post" name="form2">
  <select name="type" id="type">
    <option value="Semua">Semua</option>
    <option value="Berita">Berita</option>
    <option value="Info">Info</option>
  </select>
  <input type="submit" id="type" />
</form>

<table width="200" border="1" class="tbl_art_content" id="results">
<tbody>
  <tr>
  </tr>
    <?php
        $i=0;
        $no=0;
            if($_POST['type'] == 'Semua')
            {
                $query_art = mysql_query("SELECT * FROM artikel_tbl ORDER BY id") or die(mysql_error());
            }
            else if ($_POST['type'] == 'Berita')
            {
                $query_art = mysql_query("SELECT * FROM artikel_tbl WHERE type = 'Berita' ORDER BY id") or die(mysql_error());
            }
            else if ($_POST['type'] == 'Info')
            {
                $query_art = mysql_query("SELECT * FROM artikel_tbl WHERE type = 'Info' ORDER BY id") or die(mysql_error());
            }
            else
            {
                $query_art = mysql_query("SELECT * FROM artikel_tbl ORDER BY id") or die(mysql_error());
            }
            while($show=mysql_fetch_array($query_art))
            { 
                $no++;
                if(($no%2)==0)
                    $color = '#f2f2f2'; 
                else
                    $color = '#f9f9f9';
    ?>
  <tr bgcolor="<?php echo $color; ?>" class="rows">
    <td class="chk_content"><input type="checkbox" name="checked<?php echo $i; ?>" value="<?php echo $show['id']; ?>"/></td>
    <td class="no_content"><?php echo $no; ?></td>
    <td class="middle_content"><?php echo $show['judul']; ?></td>
    <td class="middle_content"><?php echo $show['penulis']; ?></td>
    <td class="middle_content"><?php echo $show['type']; ?></td>
    <td class="middle_content"><img src=".././upload/artikel/<?php echo $show['foto']; ?>" width="144" height="88"/></td>
    <td class="middle_content"><?php echo $show['tanggal']; ?></td>
    <td class="aksi_content"><div id="aksi_table">
            <ul>
                <li><a href="table_artikel.php?edit=<?php echo $show['id']; ?>"><img src="images/Apps-text-editor-icon.png" width="20" height="20" /></a></li>
                <li><a href="table_artikel.php?delete=<?php echo $show['id']; ?>" onclick="return confirm('Hapus artikel?')";><img src="images/Remove-icon.png" width="20" height="20"/></a></li>
            </ul>
        </div><!-- end of aksi_table --></td>
  </tr>
    <?php
        $i++; 
        }
    ?>
    <input type="hidden" name="n" value="<?php echo $i; ?>" /> 
    </tbody>
</table>
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Well... First we can shorten your if-construct a lot:

$sql = 'SELECT * FROM artikel_tbl';
if ($_POST['type'] == 'Info' || $_POST['type'] == 'Berita')
  $sql .= " WHERE type = '{$_POST['type']}'";
$sql .= ' ORDER BY id';

In my opinion this also increases readability, because it is clear that the query is basically everytime the same, you are just including a condition in one case.

Next thing the if(($no%2)==0)-part. Do you know about the nth-child of css? Here is an example for that: http://jsfiddle.net/2ZR2U/

For these things: <?php echo $show['judul']; ?> change this to read <?php echo htmlspecialchars($show['judul']); ?>. Unless you are very sure that the input is just a number or something that is guaranteed to not contain any of [<>"] this is very important to prevent display bugs or in worst case cross site scripting attacks.

\$\endgroup\$
0
2
\$\begingroup\$
  1. When you are merely "reading" data from the server/database, the appropriate way to pass data with this request is generally $_GET. $_POST is used when you are "writing" data to the server/database. Your search form should be using method="get" instead of method="post". (not that it is breaking anything, just a better practice) Using $_GET will also allow your users to bookmark the actual selection and navigate directly to specific selection which is a better UX.

  2. There is absolutely no benefit in repeating the <option>'s text as the value attribute value. It doesn't even matter if you are employing javascript processes on the element. Simply omit the value attribute declarations from all option tags.

  3. mysql_ functions should no longer exist in any code, here's why: https://stackoverflow.com/a/12860046/2943403

  4. Extending the advice in that answer to this page, you should be using a prepared statement with a placeholder for cases when you want to include user-supplied data into your query. Sure, you can validate the incoming values to ensure the safety of your query, but when you are using the best practice throughout your application, you can confidently and interchangeably copy-paste snippets from any of your projects.

  5. I recommending keeping processing code at the top of your file and all of the markup generation at the bottom. This separation should help to make your script easier to maintain.

  6. If the user did select a specific option, it will be expected to present that selection in the <select> field when the page is loaded.

  7. Declare an options array which can be used as a validating whitelist AND to populate the extra options of your select field.

  8. To alternate the styling (in this case, background color) of html table rows, use css only. This is a much more sleak technique than bloating your php code. Please see: https://stackoverflow.com/q/3084261/2943403

  9. Do not use or die(mysql_error()) in production, this will present potentially usable details about your database to "bad actors" who may attempt to infiltrate your system.

  10. aksi_table is an id value that is declared inside a loop. This is a no-no. Element id values must be unique within an html document. Unless you have another alternative solution, this attribute should be declared as a class. To be honest, I don't see any benefit in wrapping your <ul> element in a <div> container. It is best to try to minimize the number of elements that your html document needs.

  11. Your inline javascript should be translated into "listeners" which should be written in a <script> section or in an external javascript file. This will make the row template less bloated and easier to read. The same is true for the inline styling -- move anything that can be moved to a css file.

  12. I have to assume that I am not looking at all the vital code for this script. There seems to be no second form, so I don't what the purpose of the hidden input is.

  13. As stated in #1, table_artikel.php?edit= and table_artikel.php?delete= are bad ideas. These actions should be triggered by a form submission or ajax process and only with post as the method. You don't want a crawler to find your "data writing" scripts and wipe out or modify all of the data in your database table. I'll include your technique in my snippet, but you need to fundamentally change these processes.

Code:

<?php
$conn = new mysqli("localhost", "root", "", "myDB");

$options = ['Berita', 'Info'];

if (isset($_GET['type']) && in_array($_GET['type'], $options)) {
    $selected = $_GET['type'];
    $stmt = $conn->prepare("SELECT * FROM artikel_tbl WHERE `type` = ? ORDER BY id");
    $stmt->bind_param('s', $_GET['type']);
    $stmt->execute();
    $result = $stmt->get_result();
} else {
    $selected = '';
    $result = $conn->query("SELECT * FROM artikel_tbl ORDER BY id");
}

?>
<form method="get" name="form2">
    <select name="type" id="type">
        <option>Semua</option>
        <?php
            foreach ($options as $option) {
                printf(
                    "<option%s>%s</option>",
                    $selected === $option ? ' selected' : '',
                    $option
                );
            }
        ?>
    </select>
    <input type="submit" />
</form>

<?php
$rowTemplate = '
    <tr class="rows">
        <td class="chk_content">
            <input type="checkbox" name="id" value="%1$d"/>
        </td>
        <td class="no_content">%2$d</td>
        <td class="middle_content">%3$s</td>
        <td class="middle_content">%4$s</td>
        <td class="middle_content">%5$s</td>
        <td class="middle_content">
            <img src=".././upload/artikel/%6$s" width="144" height="88"/>
        </td>
        <td class="middle_content">%7$s</td>
        <td class="aksi_content">
            <div class="aksi_table">
                <ul>
                    <li>
                        <a href="table_artikel.php?edit=%1$d">
                            <img src="images/Apps-text-editor-icon.png" width="20" height="20" />
                        </a>
                    </li>
                    <li>
                        <a href="table_artikel.php?delete=%1$d" onclick="return confirm(\'Hapus artikel?\')";>
                            <img src="images/Remove-icon.png" width="20" height="20"/>
                        </a>
                    </li>
                </ul>
            </div>
        </td>
    </tr>
';
?>

<table width="200" border="1" class="tbl_art_content" id="results">
    <tbody>
        <?php
        foreach ($result as $row) {
            printf(
                $rowTemplate,
                $row['id'],       // %1$d -- used multiple times
                $i + 1,           // %2$d
                $row['judul'],    // %3$s
                $row['penulis'],  // %4$s
                $row['type'],     // %5$s
                $row['foto'],     // %6$s
                $row['tanggal']   // %7$s
            );
        }
        ?>
    </tbody>
</table>
\$\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.