PHP Developers Network

A community of PHP developers offering assistance, advice, discussion, and friendship.
 
Loading
It is currently Tue Sep 19, 2017 8:30 am

All times are UTC - 5 hours




Post new topic Reply to topic  [ 11 posts ] 
Author Message
PostPosted: Thu Apr 17, 2014 9:03 pm 
Offline
Forum Newbie

Joined: Thu Apr 17, 2014 7:46 am
Posts: 5
Hello,
I'm new to webdev and still trying to understand what to do and what not to do..
Last night I wrote a small script to help myself with keeping all the notes and ideas in one place. I'm using PHP/HTML/JS(jQuery+ajax) only, whole script is written in one file (except for jquery).
Script took me 2 hours to write, for me it looks like I have done everything correctly, but I doubt it. Could I please get some critique and tips on what (if) I'm doing wrong?

Script:
Syntax: [ Download ] [ Hide ]
<?php
$con=mysqli_connect("-","-","-","-");
if(isset($_POST["data"])){
        $data=explode(":",$_POST["data"]);
        if($data[0]==1 && preg_match('/^[0-9]*$/', $data[1]))
                if (mysqli_query($con,"DELETE FROM tracker WHERE id=$data[1]")) echo "<span style=\"color:green\">Entry with id ".$data[1]." deleted!";
        die;
}
if(isset($_POST["edit"]) && preg_match('/^[0-9]*$/', $_POST["id"])) {
        if (mysqli_query($con,"UPDATE tracker SET data='".mysqli_real_escape_string($con,$_POST["value"])."' WHERE id=$_POST[edit]")) echo $_POST["value"];
        die;
}
echo "<head>
        <link href=\"http://fonts.googleapis.com/css?family=Prociono\" rel=\"stylesheet\" type=\"text/css\">
        <script src=\"http://ajax.googleapis.com/ajax/libs/jquery/1.11.0/jquery.min.js\"></script>
        <script src=\"js.js\"></script>
        <style>
                body { background: url(http://i.imgur.com/6RNBEba.jpg) no-repeat black fixed ; color:white; }
        </style>
        <script>
                $(document).ready(function(){
                $('.edit_area').editable('tracker.php', {
                        id: 'edit',
                        name: 'value',
                        type: 'textarea',
                        cancel: 'Cancel',
                        submit: 'OK',
                        event: 'dblclick',
                        indicator: '<img src=\"http://www.appelsiini.net/projects/jeditable/img/indicator.gif\">',
                        tooltip: 'Click to edit...'
                });
                        $(\".ajax\").click(function(e){
                                var r=confirm('Are you sure you want to delete this record?');
                                if (r==true)
                                {
                                        $.ajax({
                                                type: 'POST',
                                                url: 'tracker.php',
                                                data: { data: $(this).attr('value')},
                                                success:function(result){
                                                        $('#result').html(result);
                                                        $('#saraksts').html('<font color=\"white\">Loading..</color>');
                                                        $.ajax({
                                                                type: 'POST',
                                                                url: 'tracker.php',
                                                                data: { load: 'saraksts'},
                                                                success:function(result){ $('#saraksts').html(result); }
                                                        });                                                            
                                                }
                                        });
                                }
                        });
                });
        </script>
</head>"
;
echo "<body>";
if($_POST["submit"]) mysqli_query($con,"INSERT INTO tracker (type,data,time,deleted) VALUES (".mysqli_real_escape_string($con,$_POST["type"]).", '".mysqli_real_escape_string($con,$_POST["text"])."',".time().", 0)");
if(!$_POST["load"]) echo "<center>
        <form action=\"tracker.php\" method=\"POST\">
                <select name=\"type\" style=\"color: white; background-color: #1F1F00\">
                        <option value=\"1\">Note</option>
                        <option value=\"2\">Important</option>
                        <option value=\"3\">Idea</option>
                </select><br/>
                <textarea name=\"text\" rows=\"4\" cols=\"50\" style=\"color: white; background-color: #1F1F00\"></textarea><br/>
                <input type=\"submit\" name=\"submit\" value=\"Submit\" style=\"color: white; background-color: #1F1F00\">
        </form>
        <div id='result'></div>
</center>"
;
$t=mysqli_query($con,"SELECT * FROM tracker WHERE deleted=FALSE ORDER BY time DESC"); if(mysqli_num_rows($t)){
        echo "<div id=\"saraksts\" align='center' style='width:800px; margin: auto'><table style=\"width:100%;border-collapse: collapse; \">";
        $c; $cc;
        while ($t2 = mysqli_fetch_array($t)) {
        if($c) { $c=FALSE; $cc="#191919"; } else { $c=TRUE; $cc="#303030"; }
        if($t2[2]==1) $ccc="#0066FF"; elseif($t2[2]==2) $ccc="#FF0000"; elseif($t2[2]==3) $ccc="#00FF00";
        echo "<tr>
                <td style=\"background-color:"
.$ccc.";width:0.5%;\"></td>
                <td style=\"background-color:#1F1F00; width:20%; color:white; border-bottom:dashed grey; border-width:1px;\">
                        <a class='ajax' value='1:"
.$t2[1]."'>[<span style=\"color:red\">X</span>]</a> ".date("Y/m/d H:i:s",$t2[4])."
                </td>
                <td style=\"color:white; font-family: 'Prociono', serif; border-bottom:dashed grey; border-width:1px; background-color:"
.$cc.";opacity:0.85;\">
                        <div id='"
.$t2[1]."' class=\"edit_area\">".str_replace("\\", "", $t2[3])."</div>
                </td>
        </tr>"
;
        }
        echo "</table></div></body>";
}

I wrote it for personal use only.


Last edited by SaW1337 on Mon Apr 28, 2014 9:29 am, edited 1 time in total.

Top
 Profile  
 
PostPosted: Sat Apr 19, 2014 4:48 pm 
Offline
Moderator
User avatar

Joined: Tue Nov 09, 2010 3:39 pm
Posts: 6345
Location: Montreal, Canada
Some quick observations, point form.
  • No HTML tags? No doctype declaration? No charset specified?
  • You're echoing everything. Why? Don't do that.
  • As much as possible, avoid inline styles.
  • Escaping your inputs is good. Prepared statements are better.
  • Use braces. Always.
  • Does this really all belong in the same file?


Top
 Profile  
 
PostPosted: Mon Apr 21, 2014 10:50 am 
Offline
Forum Newbie

Joined: Thu Apr 17, 2014 7:46 am
Posts: 5
  • You're echoing everything. Why? Don't do that. I thought it's better than closing/opening <?php ?> all the time..
  • Use braces. Always. I thought you don't need to open a brace for single function.. Increases readability. Could you please explain how is this bad?
  • Does this really all belong in the same file? I usually don't do that but this time script was kinda small and it felt unnecessary to make includes for 5 line codes

Thank you for the tips :)


Top
 Profile  
 
PostPosted: Mon Apr 21, 2014 11:10 am 
Offline
Moderator
User avatar

Joined: Tue Nov 09, 2010 3:39 pm
Posts: 6345
Location: Montreal, Canada
SaW1337 wrote:
You're echoing everything. Why? Don't do that. I thought it's better than closing/opening <?php ?> all the time..

I think it hurts readability. You lose syntax highlighting, making mistakes harder to spot. Much easier to break by forgetting to escape a double quote somewhere. I keep my views and templates bereft of PHP save for flow control.

SaW1337 wrote:
Use braces. Always. I thought you don't need to open a brace for single function.. Increases readability. Could you please explain how is this bad?

You don't technically, as evidenced by the fact that your script is working fine. Adding a second line inside the conditional and not noticing the missing braces can potentially lead to a whole bunch of frustration. Those two extra characters don't really cost anything but can save some headaches down the road. Also, I find the braces improve readability, though that's a lot more subjective.


Top
 Profile  
 
PostPosted: Thu Jul 10, 2014 6:55 pm 
Offline
Forum Newbie

Joined: Thu Apr 17, 2014 7:46 am
Posts: 5
Hello again.. Been reading and learning PHP for some time now.
I made login and autologin 2 in 1 (for my upcoming script). I still don't use braces for single function, somehow I don't like it and it improves readability for me.

What makes me wonder is..
1) Is this script safe (I know security is an illusion, but against majority of hackers)?
2) I'm storing logins in MySQLi Engine=MEMORY (ram) so it reads login faster.. is this a right thing to do, or is mysql caching good enough?

Script: http://pastebin.com/eqw42TMd (no html)


Top
 Profile  
 
PostPosted: Thu Jul 10, 2014 7:16 pm 
Offline
Moderator
User avatar

Joined: Tue Nov 09, 2010 3:39 pm
Posts: 6345
Location: Montreal, Canada
Syntax: [ Download ] [ Hide ]
$password=isset($_POST['password']) ? $_POST['password'] : $_SESSION["password"];
...
mysqli_prepare($con, "SELECT u_id FROM accounts WHERE u_username=? AND u_password=? LIMIT 1")
...
mysqli_stmt_bind_param($sql, "ss", $username, $password);

Looks like you're storing passwords in plain text. If your database is ever compromised, you have potentially hurt your users. People shouldn't use the same password on multiple sites, but they do. Hash your passwords. Use bcrypt.

Syntax: [ Download ] [ Hide ]
if(!ctype_alnum($username.$password)) $err[].="Username and Password must contain only alphanumeric symbols!";

You also don't want to put unnecessary restrictions on passwords. Requiring a digit and a special character? OK. Makes sense. Forbidding special characters reduces the number of possible passwords, making cracking them easier.


Top
 Profile  
 
PostPosted: Thu Jul 10, 2014 10:32 pm 
Offline
Site Administrator
User avatar

Joined: Wed Aug 25, 2004 7:54 pm
Posts: 13427
Location: New York, NY, US
One of the biggest things you should do is separate your Domain code from your Presentation code. I would recommend putting all the code the loads/saves data to the tracker table into a Tracker or TrackerModel class. It should have methods like insert(), update(), delete() and findAvailable() (not sure what deleted=FALSE means!?). Then have your page templates access that object. It will keep all code together for reuse, refactoring, etc.

_________________
(#10850)


Top
 Profile  
 
PostPosted: Fri Jul 11, 2014 5:18 am 
Offline
Forum Newbie

Joined: Thu Apr 17, 2014 7:46 am
Posts: 5
Celauran wrote:
Syntax: [ Download ] [ Hide ]
$password=isset($_POST['password']) ? $_POST['password'] : $_SESSION["password"];
...
mysqli_prepare($con, "SELECT u_id FROM accounts WHERE u_username=? AND u_password=? LIMIT 1")
...
mysqli_stmt_bind_param($sql, "ss", $username, $password);

Looks like you're storing passwords in plain text. If your database is ever compromised, you have potentially hurt your users. People shouldn't use the same password on multiple sites, but they do. Hash your passwords. Use bcrypt.

Syntax: [ Download ] [ Hide ]
if(!ctype_alnum($username.$password)) $err[].="Username and Password must contain only alphanumeric symbols!";

You also don't want to put unnecessary restrictions on passwords. Requiring a digit and a special character? OK. Makes sense. Forbidding special characters reduces the number of possible passwords, making cracking them easier.

Thanks for tips, will look into bcrypt and will put much less restrictions on passwords.

Christopher wrote:
One of the biggest things you should do is separate your Domain code from your Presentation code. I would recommend putting all the code the loads/saves data to the tracker table into a Tracker or TrackerModel class. It should have methods like insert(), update(), delete() and findAvailable() (not sure what deleted=FALSE means!?). Then have your page templates access that object. It will keep all code together for reuse, refactoring, etc.

I'm now doing PHP first and template only later, as an include (as you can see, there's no HTML in my second code).I don't understand about Tracker/TrackerModel class.. Is that some kind of framework? Also, if that's something to do with OOP - I'm trying to avoid it. I spent hours researching both Procedural and OOP, came to an conclusion that 99.9% of the time OOP is not necessary and it only slows down compiling times. I don't think I will ever need those special features OOP offers.. OOP vs Pro. syntax is purely subjective and I like Pro. better, but I can understand why some people prefer OOP (readability, organized, etc.)


Top
 Profile  
 
PostPosted: Fri Jul 11, 2014 1:13 pm 
Offline
Site Administrator
User avatar

Joined: Wed Aug 25, 2004 7:54 pm
Posts: 13427
Location: New York, NY, US
SaW1337 wrote:
Also, if that's something to do with OOP - I'm trying to avoid it. I spent hours researching both Procedural and OOP, came to an conclusion that 99.9% of the time OOP is not necessary
You are incorrect in your conclusion and are simply limiting yourself to a subset of the language syntax to your detriment. But you will prove that to yourself the hard way in the long run (trust me).
SaW1337 wrote:
and it only slows down compiling times.
PHP is not compiled and you will not notice any performance difference on any site you will build.
SaW1337 wrote:
I don't think I will ever need those special features OOP offers.. OOP vs Pro. syntax is purely subjective and I like Pro. better, but I can understand why some people prefer OOP (readability, organized, etc.)
They are not special features and it is not purely subjective. Conversely almost all the problems with the design of your code are due to the lack of use of OOP. And yes, people always argue that anything you can do in OOP you can do procedurally. But there are no major PHP frameworks or applications that are procedural only. It is just not done because it is so inferior.

_________________
(#10850)


Top
 Profile  
 
PostPosted: Sat Jul 12, 2014 8:34 am 
Offline
Forum Newbie

Joined: Thu Apr 17, 2014 7:46 am
Posts: 5
Could you please explain further and maybe give some example or some article? If you're correct, my findings about OOP were horrible and maybe I really should learn it.


Top
 Profile  
 
PostPosted: Sat Jul 12, 2014 12:42 pm 
Offline
Site Administrator
User avatar

Joined: Wed Aug 25, 2004 7:54 pm
Posts: 13427
Location: New York, NY, US
You don't need to go OOP crazy or be an OOP zealot (please don't), but limiting yourself to only part of a language's syntax -- especially a very useful part -- is unwise. At its simplest, the class construct is a nice way to namespace functions. Objects are essentially data structures with a namespace of functions that go with them. It is very useful to pass around data structures instead of having lots of individual variables littering your code. And since the advent of OO, there are a bunch of useful design concepts (and may reusable good design practices called patterns) that OO enables. These simply expand the available tools you can use to solve software problems. There's neither magic nor politics behind OO -- they are just practical part programming.

_________________
(#10850)


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 11 posts ] 

All times are UTC - 5 hours


Who is online

Users browsing this forum: No registered users and 2 guests


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Jump to:  
Powered by phpBB® Forum Software © phpBB Group