PHP: Making cleaner & shorter code

Question

I have this:

<?php
if($_GET['wE'] && is_numeric($_GET['wE'])){
$we = mysql_real_escape_string($_GET['wE']);
$query_find_we = "SELECT id FROM users_wall WHERE id = '$we' AND uID = '$showU[id]'";
$query_find_we = mysql_query($query_find_we)or die(mysql_error());
$grab_wall_comment = (mysql_num_rows($query_find_we) == 1) ? "window.location.hash = '#comment$we';" : "alert('Vägginlägg kunde ej hittas.');";
?>
<script>
$(function() {
<?php echo $grab_wall_comment; ?>
});
</script>
<?php
}elseif($_GET['sE'] && is_numeric($_GET['sE'])){
$se = mysql_real_escape_string($_GET['sE']);
$query_find_se = "SELECT id FROM users_statuslog WHERE id = '$se' AND uID = '$showU[id]'";
$query_find_se = mysql_query($query_find_se)or die(mysql_error());
$grab_status_comment = (mysql_num_rows($query_find_se) == 1) ? "window.location.hash = '#comment$se';" : "alert('Status kunde ej hittas.');";
?>
<script>
$(function() {
<?php echo $grab_status_comment; ?>
});
</script>
<?php
    }
?>

checks if any $_GET['we'] or $_GET['se'], and if it exists. and runs the <script>.

Is there a way to make this shorter? I tried myself alittle with making a boolean, but maybe you could short it even more? Any ideas, as I would like to make cleaner coding in the future..

please answer and provide example of how you mean by wrapping it in a function. — Karem, Dec 05 '10 at 16:03
first of all you have to learn proper code indentation. Your current code is totally unreadable. — Your Common Sense, Dec 05 '10 at 17:24

score 1 · Accepted Answer · edited May 23 '17 at 11:47

1

Don't you see it yourself?

There are obviously 2 identical parts of code.
You have to just make variable parts of this code into PHP variables. That's all.
And of course there should be some db API function in use.

<?
if (!empty($_GET['wE'])) {
  $id = $_GET['wE']
  $table = "users_wall";
  $alert = "Vagginlagg kunde ej hittas.";
} elseif (!empty($_GET['sE'])) {
  $id = $_GET['sE'];
  $table = "users_statuslog";
  $alert = "Status kunde ej hittas.";
}
$query = "SELECT count(id) FROM `$table` HERE id = %d AND uID = %d";
$count = db::getOne($query,$id,$showU['id']);

//Separate your main PHP logic from presentation as much as possible.
//leave only necessary operators.
?>
<script>
$(function() {
<? if($count): ?>window.location.hash = '#comment<?=$id?>';
<? else: ?>alert('<?=$alert?>');
<? endif ?>
});
</script>

getone() function is similar to getarr() I mentioned in this answer but returns a scalar value instead of array.

edited May 23 '17 at 11:47

Community

1
1

answered Dec 05 '10 at 18:01

Your Common Sense

156,878
40
214
345

"Separate your main PHP logic from presentation as much as possible." - how about putting it in a different file. – Kendall Hopkins Dec 05 '10 at 18:26
@Kendall no problem. Just cut everything below comments into separate file and place include call instead. – Your Common Sense Dec 05 '10 at 18:36
About your comment in the answer, I am using notepad++ could this be why my code is unreadable? Or what did you mean by that? – Karem Dec 05 '10 at 21:51
@Karem not. it's your job to properly indent code, no editor will do it for you (however, good editor always keep an indent from previous line). – Your Common Sense Dec 06 '10 at 05:29
Ok thank you. I started using PDO now, what should I use instead of db::getOne? – Karem Dec 07 '10 at 15:17
@Karem getOne is a user function. You can use not only ready-made functions, but also create one of your own. So, getOne is of this kind. It has nothing to do with PDO or other library. You can make it using PDO as well. But it seems it's far too complicated task for you, so - nevermind. – Your Common Sense Dec 08 '10 at 07:07

score 0 · Answer 2 · answered Dec 05 '10 at 16:32

<?php
    $param = (isset($_GET['wE']) && is_numeric($_GET['wE']))?"wE":((isset($_GET['sE']) && is_numeric($_GET['sE']))?"sE":false);
    if($param){
        echo showScript($param);
    } else {
        //TODO: do something if there's no 'wE' nor 'sE' maybe?
    }

    function showScript($param){
        if($param == "wE"){
            $table = "users_wall";
            $alert = "Vägginlägg kunde ej hittas.";
        } else {
            $table = "users_statuslog";
            $alert = "Status kunde ej hittas.";
        }
        $e = mysql_real_escape_string($_GET[$param]);
        $query_find = "SELECT id FROM ".$table." WHERE id = '".$e."' AND uID = '$showU[id]'";
        $query_find = mysql_query($query_find)or die(mysql_error());
        $grab_status_comment = (mysql_num_rows($query_find) == 1) ? "window.location.hash = '#comment$e';" : "alert('$alert');";
        return '<script>$(function() {'.$grab_wall_comment.'});</script>';
    }
    ?>

i haven't tested it... but that's the main idea :P

good luck, hope this helps

PHP: Making cleaner & shorter code

2 Answers2

Linked