nokonoko / Pomf

Simple file uploading and sharing, source for the now shut down site Pomf.se
MIT License
463 stars 98 forks source link

changed name generation to a for loop #21

Closed Battleroid closed 9 years ago

Battleroid commented 9 years ago

Change the repetition of adding a random character six times to a for loop.

SEAPUNK commented 9 years ago

already discussed in here

phillid commented 9 years ago

I've only just met this project thanks to Reddit, and personally, I don't see anything wrong with this pull request.

I acknowledge that the code this pull request touches is going to be rewritten (cite).

I feel the harmlessness (well, helpfulness) of this request is overruling, in order to maintain the best code quality (I'm chiefly thinking scalability here) in the meantime. Considering comments about harmlessness from @NuckChorris, this pull request could surely be merged?

Furthermore, we should note the line containing $newname = '';. Initialisation of this variable is only required if it would otherwise be referenced before assignment, i.e. during concatenation within a loop. When poor code style is embraced and six separate statements are used, the logical method would be for initialisation to come in the form of an assignment of the first random character, with the next five characters being concatenations. I should conclude that this line of code is surely evident of this code having originally been written as a loop, or intended to be packed into a loop?

My two cents.

NuckChorris commented 9 years ago

Can't disagree with the initialization being pointless. This was originally written as a chunk of the crc32 + some letters and numbers. Not a loop, more of a horror story.

It's unrolled because, as I state in the other thread "this is replacing a more obvious (but perhaps uglier and harder to change) structure with a less obvious version."

A full-fledged for loop clutters the mental model and forces you to actually unroll the loop by going down and repeating that wait how many oh six times. Your eye will skip around the loop and it will take longer. You need to process the contents of the for loop and figure out how that exact combination of numbers and symbols will loop. Backwards or forwards? How many times? Off-by-one issues? Who knows.

All of which doesn't happen when it's unrolled.

And this is 6 fucking lines.

phillid commented 9 years ago

First of all, I acknowledge that it's 'only' six lines, but this code is public, and is open for public inspection, for people to learn programming practice from, and so on.

I'd hate for us to venture near anything resembling an argument. I can fully sympathise with your argument about a loop cluttering the mental model, only if the loop's variable was being passed or used within the loop.

I'm sure you've iterated over arrays thousands of times before, in which case, off-by-one errors simply don't happen. Start at zero, stop just before six, you'll have iterated six times. It's bog-standard.

repeating that wait how many oh six times

In my humble opinion, this is exactly what you have to do when reading this code as it stands. A quick glance and my mind separated the lines into a group of two and one of three. I manually counted, and confirmed that there were -- whoops -- I miscounted, it's six characters, not five! Off by one :) Then again, maybe that's just me. Oh, and the loop direction doesn't matter either. It'd be easily identified as a simple concatenation to form a string and instantly recognised that backwards or forwards has no effect on this.

Then again, it's your repo.

nokonoko commented 9 years ago

Jesus christ the autism, however I applied this pull https://github.com/nokonoko/Pomf/pull/24 since you could select which chars you wanted thus making it a bit easier for people not knowing any PHP at all to modify the name generation.

And again, it's 6 lines as Nuck said, it does not make any big impact on performance at all, tested both of the codes and the new one is a bit faster with under a microsecond, LESS THEN A FUCKING MICROSECOND.

Then again, it's your repo.

No it's mine, so can't really put the blame on Nuck, he is however trying to debate the logic in why it's there unrolled, but you people sure are dense.

Battleroid commented 9 years ago

I'm going to pet my cat and go to bed.