How not to maintain a news form



  • Fear my horrible code that doesn't remember what category it's supposed to be using for some reason. Although if someone can tell my why it doesn't I'll be eternally grateful. I have actually now rewritten this from scratch.

    <?php
        
    if ($_GET['id'] !== NULL) {
            
    validate_num($_GET['id'], 'id');
        }
        if (
    $_GET['id2'] !== NULL) {
            
    validate_num($_GET['id2'], 'id2');
        }
        
        if (isset(
    $_GET['id'])) $id $_GET['id'];
        if (isset(
    $id) AND preg_match ("/^([0-9]+)$/"$id)) {
            
    $sql 'SELECT  FROM '.TABLE_NEWS.' WHERE news_id = '.$id;
            
    $query $db->sql_query($sqlFILELINE);
            if ((
    $row $db->fetch_array($query)) !== false) {
                
    $text = ($row['news_details']);
                
    $extra = ($row['news_extra']);
                
    $name = ($row['news_name']);
                
    $column $row['news_cat'];
                
    $act $row['news_cat'];
            } else {
                echo 
    'Invalid ID: '.$id;
            }
            unset(
    $query);
        } else {
            
    $text firstslashes($_POST['text']);
            
    $extra firstslashes($_POST['extra']);
            
    $name firstslashes($_POST['name']);
            
    $column $_POST['column'];
            
    $act $_POST['act'];
        }
        
    $act = ($act) ? 0;
        
    $column = ($column) ? intval($_POST['column']) : 1;
        if (
    $act == 0) {
            
    $column 0;
        }
        if (
    $_POST['sub'] && strlen($name) > && isset($column) && strlen($text) > 1) {
            if (!isset(
    $_GET['id2'])) {
                
    $sql2 'INSERT INTO '.TABLE_NEWS.' ( news_namenews_detailsnews_extranews_catnews_datenews_act)'
                    
    .' VALUES ( ''.$name.'', ''.$text.'', ''.$extra.'', '.$column.', '.time().', '.$act.' );';
            } else {
                
    $sql2 'UPDATE '.TABLE_NEWS.' '
                    
    .'SET news_name = ''.$name.'', news_details = ''.$text.'', news_extra = ''.$extra.'', news_cat = '.$column.', '
                    
    .'news_act = '.$act
                    
    .' WHERE news_id = '.$_GET['id2'].';';
            }
            
    $db->sql_query($sql2FILELINE);
            if (isset(
    $_GET['id2'])) {
                echo 
    'Updated!<br />';
            } else {
                echo 
    'Posted!<br />';
            }
        } else if (
    $_POST['sub']) {
            
    $_POST['text'] = htmlentities($_POST['text']);
            echo 
    '<br />You are missing:<br /><br />';
            if (!
    $_POST['name']) echo 'Title<br />';
            if (!
    $_POST['column']) echo 'Category<br />';
            if (!
    $_POST['text']) echo 'Text<br />';
        }
        
    $sql3 'SELECT 
     FROM '
    .TABLE_CAT.';';
        
    $query $db->sql_query($sql3FILELINE);
        while ((
    $row $db->fetch_array($query)) !== false) {
            if (
    is_array($news) AND $news[0] != AND !in_array($row['cat_id'], $news)) continue;
            if (
    $row['cat_id'] == $column) { $check ' checked="checked" selected="selected"'; }
            else { unset(
    $check); }
            
    $checks[] = '<input type="radio" name="column" value="'.$row['cat_id'].'"'.$check.' /> &nbsp; '.$row['cat_name'].'<br />';
        }

        if (
    $_POST['act'] || !isset($id)) { $check ' checked="checked" selected="selected"'; }

        if (isset(
    $_GET['id2'])) $id $_GET['id2'];
        
    $idp = (isset($id)) ? '&amp;id2='.$id '';
        echo 
    '<form method="POST" action="'.FILE.'?action=news'.$idp.'">
                <input name="name" type="text" value="'
    .stripslashes($name).'" size="45"><br>';
        
    array_to_table($checks'''left');
        echo 
    '<textarea name="text" cols="110" rows="15">'.stripslashes($text).'</textarea><br>
                Extra Information:<br />
                <textarea name="extra" cols="110" rows="15">'
    .stripslashes($extra).'</textarea><br>
                <input type="checkbox" name="act" value="1"'
    .$check.'> &nbsp; Activate News (Disabling this hides the news)<br />
                <input type="submit" name="sub" value="Submit">
            </form>'
    ;
    ?>



  • My mother told me that if I didn't have something nice to say, I shouldn't say anything at all. And boy do I ever have a lot of nothing to say...



  • If you have rewritten it, then what is the point of knowing where it fails?



  • @dhromed said:

    If you have rewritten it, then what is the point of knowing where it fails?

     Not having to rewrite a page because of the same mistake ?



  • TABLE_CAT



  • ExcessiveColourException at forums.thedailywtf.com/forums, post 130043



  • @Zecc said:

    ExcessiveColourException at forums.thedailywtf.com/forums, post 130043
    You object to syntax coloring?



  • I object to red, green and blue syntax coloring. Specifically <font color="#ff0000">red</font>.

    Syntax coloring is useful for mental parsing, but not when it <font color="#ff0000">hurts</font> <font color="#00cc00">the</font> <font color="#0000ff">eyes</font> <font color="#00ff00">and </font><font color="#ff0000">confuses</font> <font color="#0000ff">the </font><font color="#ff0000">brain</font>. Mine, anyway.



  • I will change my syntax colouring to use greyscale tones from now on.



  • I must be crazy to actually react to this, but since my eye fell on it...

                 $column $row['news_cat'];
     
    [...snip...]
        } else {
    [...snip....]

            
    $column $_POST['column'];

    [...snip...]
        $column = ($column) ? intval($_POST['column']) : 1;

     

    Now, that's never going to work. Obviously. 



  • @dhromed said:

    I will change my syntax colouring to use greyscale tones from now on.

    I'm just saying you don't need agressive colors, that's all.  E.g. http://www.movabletype.org/2007/06/syntax_highlighting.html

     The font type makes it worse, I guess.
     



  • @Zecc said:

    @dhromed said:

    I will change my syntax colouring to use greyscale tones from now on.

    I'm just saying you don't need agressive colors, that's all.  E.g. http://www.movabletype.org/2007/06/syntax_highlighting.html

     The font type makes it worse, I guess.

    I find any attempt to use syntax colouring on a white background is highly likely to make a mess. It works vastly better on grey or black - something about relative contrast, I never did pay much attention in colour theory lectures. It's also easier on the eyes, since it radiates less energy.

    Of course, the best approach is to use both foreground and background colours. For example, if you colour comments and strings by using different shades of grey for the background (and normal foreground text), then it's both really obvious and really easy to read, you don't have huge colour splashes all over the place (since those two make up the vast majority of the coloured text in typical code), and you've got two more colours to use for important things. As an added bonus, embedded $vars in languages like perl colour much more clearly.



  • Case in point: [URL=http://img102.imageshack.us/my.php?image=untitleduu9.png][IMG]http://img102.imageshack.us/img102/4516/untitleduu9.th.png[/IMG][/URL]

    I prefer (by default, because I've never tried otherwise) a white background. I keep the luminosity and monitor low.

    I never use light colors for text, except eventually comments.

     

    And that's enough discussion of the topic for me. 



  • Double post

    I keep the luminosity and contrast of the monitor low.

    Wretched edit timeout. Thwarted me again!



  • @dhromed said:

    If you have rewritten it, then what is the point of knowing where it fails?

    The entire thing is a flurry of mistakes, and the column forgetting is the only mistake I haven't figured out, which means I am more likely to repeat it.



    As for the colors, sorry, I won't pipe the code (presuming it's PHP again) through highlight_file, although the output it produces is kinda a wtf of it's own.



  • @Zecc said:

    Case in point: [URL=http://img102.imageshack.us/my.php?image=untitleduu9.png][IMG]http://img102.imageshack.us/img102/4516/untitleduu9.th.png[/IMG][/URL]

    I prefer (by default, because I've never tried otherwise) a white background. I keep the luminosity and monitor low.

    I never use light colors for text, except eventually comments.

     

    And that's enough discussion of the topic for me. 

     

    I prefer a grey background, with black text, and muted colors for highlighting.  It'll save your eyes when you have to stare at the damned thing for 8 hours a day... 



  • Wait, don't tell me... You use the same colours as this site so that people don't notice when you're procrastinating. 🙂



  • @Zecc said:

    Wait, don't tell me... You use the same colours as this site so that people don't notice when you're procrastinating. 🙂

     

    Shhh... you'll blow my cover. 



  • So... let's see.

     

    1. if ($_GET['id'] !== NULL)

    why not just use isset($_GET['id']) like you do a few lines later? Exactly the same thing and far more readable with isset().

    2. if (isset($_GET['id'])) $id = $_GET['id'];

    You just did this test earlier. Go for:

    if (isset($_GET['id'])) {
       validate_num($_GET['id'], 'id');
       $id = $_GET['id'];
    }

    unless validate_num() is doing something funky like overwriting what's in $_GET, which I see you do for other values elsewhere.

    3. You're blindly assuming that if $_GET['id'] isn't set, you're doing a POST and slurping data from $_POST.

    how about:

    if ($_SERVER['REQUEST_METHOD'] != 'POST') {
      .... do get stuff here ....
    } else {
      .... do post stuff here ...
    }

     
    4. if ($_POST['sub']] && etc...
         if (!isset($_GET['id2'])

    Generally $_GET will be empty if you're doing a post, unless you're passing GET parameters in the <form action="script.php?yada=yada"> tag, which is weird. Why not just embed whatever you're passing as hidden fields inside the post form?

    5. $_POST['text'] = htmlentities($_POST['text']);

    Don't overwrite contents of $_POST (or _GET or _REQUEST for that matter). It's bad form. Just save whatever you're doing into an intermediate variable and use that wherever you have to. 

    6. $text = firstslashes($_POST['text']);

    Not a standard function, what's it doing? Undoing magic_quotes_gpc? Escaping stuff for later insertion into the DB, which you're doing a few lines later? For one, depending on magic_quotes_qpc being on is STUPID. What if this runs on a host where it defaults to off? Don't write your own escape functions either. I don't recognize which DB library you're using there, but any such library that's worth not shooting in the head will have its own escape function already there.

    7. $sql2 = 'INSERT INTO'.TABLE_NEWS.' (etc...) .'VALUES ('''.$name.''. '\ etc...

    Learn how to use heredocs. This is FAR FAR FAR more readable as

    $x = TABLE_NEW; 

    $sql = <<<EOL
    INSERT INTO $x VALUES (...fields...) VALUES ($name, $text, etc...)
    EOL

    or better yet, use prepared statements, if your DB library supports that.

    8. At one point you do $text = firstslashes(), and later on do stripslashes($text). If $text gets to be large, then you're doing, and UNDOING, a large amount of work. If you're going from state A to state B, and then back to state A, keep a copy of the original state A and save state B into a different variable. That way you only do the work once, and everyone's happy. Unless text is so large you can't afford the extra memory usage, of course.


    9. As for the rest, do your own homework 🙂



  • I may as well try and explain why some things are as they are. Thank-you MarkB for spending the time to write something that thorough. Also thank-you Monomelodies for noticing that categories issue.



    > why not just use isset($_GET['id']) like you do a few lines later? Exactly the same thing and far more readable with isset().

    This was written hack after hack; I was the only coder so I didn't follow any standard, also this was actually one of my first programs.

    I agree though, I should have really refactored this.



    > You're blindly assuming that if $_GET['id'] isn't set, you're doing a POST and slurping data from $_POST.

    > Generally $_GET will be empty if you're doing a post, unless you're passing GET parameters in the <form action="script.php?yada=yada"> tag, which is weird. Why not just embed whatever you're passing as hidden fields inside the post form?

    Undocumented: If _GET['id'] is set then it's editing an old post, other-wise it's posting a new post or editing that new post if an error occurred.

    > Not a standard function [firsdtslashes()], what's it doing? Undoing magic_quotes_gpc? Escaping stuff for later insertion into the DB, which you're doing a few lines later?

    Actually first slashes is basically: return (if magic_quotes_gpc is on) ? $str : addslashes($str);

    stripslashes is called later because the data it's called on has been escaped already,for insertion into the DB, but I need to display it in the form because it's either for editing or fixing an error.

    > For one, depending on magic_quotes_qpc being on is STUPID. What if this runs on a host where it defaults to off? Don't write your own escape functions either.

    My test server has it as off because I don't always put GET and POST data into a DB, sometimes I put it into a file where having those slashes is annoying. The server it was built for has it on.

    >I don't recognize which DB library you're using there, but any such library that's worth not shooting in the head will have its own escape function already there.

    Homebrew, it sucks horrible chunks. It's actually the one that calls the error system (a die wrapper really), hence why it needs FILE and LINE. I use MDB2 now.

    > or better yet, use prepared statements, if your DB library supports that.

    Didn't know they existed at the time. I just recently got a book that taught me about PEAR::DB which is apparently usurped by MDB2.





    Also validate_num is probably a mini-wtf on it's own (it's like 10 lines iirc), it will die if it doesn't receive a number as the first parameter. Since I don't have immediate access to all my old source (I have more hard-drives than I have computers capable of reading them all, each one is partitioned at least thrice). I really need to start labeling my stuff.


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.