pereorga / minimalist-web-notepad

Minimalist Web Notepad
https://notes.orga.cat
1.01k stars 276 forks source link

Slightly improved comments #27

Closed qex closed 6 years ago

qex commented 6 years ago

I'm kinda like these comment, should we add them back?

qex commented 6 years ago

I've made another commit, it is a small trick equals to strlen() !== 0 but faster.

I'm not sure if it will harmful to readability, just use it as you like.

pereorga commented 6 years ago

I thought it was ok to leave it without that comment, the code already speaks here. But could be merged too, both options would be OK IMO.

Regarding the isset() micro-optimization, I don't think it's worth it. I like micro-optimizations when they make the code cleaner, or when they are inside code that runs often (i.e. thousands of times per second).

If if it was about readability, that could better that the current one, but more verbose:

if (str_len($_POST['t']) > 0) {
...

I think it's ok how it is done now, but that's again subjective.

qex commented 6 years ago

Well, I removed the commit of strlen modification and add a new one.

I'd prefer write that comment saying "only alphanumerics are allowed", it is more visual than regular expresstion.

pereorga commented 6 years ago

merged

qex commented 6 years ago

Yeah thank you. The code looks very comfortable for me now.

qex commented 6 years ago

The hash generation section looks still a little compact.

How about change

    // Initially based on http://stackoverflow.com/a/4356295/1391963
    // Do not generate ambiguous characters. See http://ux.stackexchange.com/a/53345/25513
    $characters = '23456789abcdefghjkmnpqrstuvwxyzABCDEFGHJKMNPQRSTUVWXYZ';
    $random_string = '';
    for ($i = 0; $i < $name_length; ++$i) {
        $random_string .= $characters[mt_rand(0, strlen($characters) - 1)];
    }

into

    // Initially based on http://stackoverflow.com/a/4356295/1391963
    // Do not generate ambiguous characters. See http://ux.stackexchange.com/a/53345/25513
    $characters = '23456789abcdefghjkmnpqrstuvwxyzABCDEFGHJKMNPQRSTUVWXYZ';

    for ($i = 0, $random_string = ''; $i < $name_length; ++$i) {
        $random_string .= $characters[mt_rand(0, strlen($characters) - 1)];
    }

?

nothing changed when execute, just for aesthetic feeling of code.

pereorga commented 6 years ago

I don't like the idea of initializing that variable in the for loop. I like to initialize variables there when they are only used inside the loop.

That could be another alternative, less efficient:

$characters = '23456789abcdefghjkmnpqrstuvwxyzABCDEFGHJKMNPQRSTUVWXYZ';
$random_string = '';
while (strlen($random_string) < $name_length) {
    $random_string .= $characters[mt_rand(0, strlen($characters) - 1)];
}
pereorga commented 6 years ago

For the PHP code, I think we are getting into a point where enhancements are difficult to make, or a matter of taste.

pereorga commented 6 years ago

I'm also unsure if the variable $name is needed. We could just use $_GET['f']

qex commented 6 years ago

Realize functions is easy, however make code beautiful is hard.

We always need to find a balance between efficiency, readability and adaptability.

So I am not make this a new PR, just a discussion.

Variable $characters is only used inside the loop, but it is strange to initialize it in for.

And I think $name is needed, I considered about this before.

$name is just like an alias, it gives $_GET['f'] a meaning, not just a one letter query string, it is name.

pereorga commented 6 years ago

Variable $characters is only used inside the loop, but it is strange to initialize it in for.

Yes, and that would make it less obvious to add/remove characters to the list.

$name is just like an alias, it gives $_GET['f'] a meaning, not just a one letter query string, it is name.

Originally, I used f to mean filename, because it was shorter. Maybe $_GET['filename'], $_GET['notename'] or just $_GET['note'] could be used instead. The same with $_POST['t'], where t means text. Changing that would mean more characters to transfer on every request, but would make the code more explanatory, and we could get rid of $name. Should I create a PR for that?