4 ms·

for an MVP this is fine :D Seeing how long mtgox has been around this is absurd. 1700+ lines would spin my head pretty fast.. And inline SQL is non-forgivabl

for an MVP this is fine :D Seeing how long mtgox has been around this is absurd. 1700+ lines would spin my head pretty fast.. And inline SQL is non-forgivable.

davidw 13y ago
> And inline SQL is non-forgivable What do PHP folk typically do for a big, hairy query that isn't really doable via an ORM? Not that this is the case here, just curious. I haven't touched PHP for years.
- stingraycharles 13y ago
  It's always better to use prepared statements, since it prevents SQL injection attacks. If you do not use neither an ORM or prepared statements, you either have to with trusted input, or need to sanitize that input really well (to the point it's often easier to just use prepared statements).
- easy_rider 13y ago
  Oops, "inline SQL" is probably a wrong way of putting it. Actually, even in Symfony2 which has an ORM (Doctrine), I sometimes find myself writing SQL queries in code because Doctrine can be so inefficient in some situations. Writing pure SQL makes your code less portable (i.e. tied to a specific database, in this case MySQL). Now I don't think its bad at all to have somewhat complex group/(sub)queries written this way if portability is not a big concern, and if the number of those queries is not as big, it is something you just need to be aware of. What we're actually talking about here which is really bad is string concatenation without parameterization. Meaning, it seems the code is not using a database driver. PDO has been the accepted way of doing this. You pass the parameters to PDO separately from the query for sanitation. We can all see why this is bad: if (isset($_GET['limit'])) { $limit = (int)$_GET['limit']; if ($limit < 1) $limit = 1; if ($limit > 10000) $limit = 10000; } $req = 'SELECT * FROM `Money_Bitcoin_Node` WHERE `Status` = \'up\' AND `Last_Checked` > DATE_SUB(NOW(), INTERVAL 6 HOUR) AND `Version` >= 31500 AND (`Last_Down` IS NULL OR `Last_Down` < DATE_SUB(NOW(), INTERVAL 2 WEEK)) AND `First_Seen` < DATE_SUB(NOW(), INTERVAL 2 WEEK) ORDER BY RAND() LIMIT '.$limit;
  - Nilzor 13y ago
    Exactly. Parameterization is #1 priority. Second you could also move the statement string itself to a separate resource (disk file), which helps for maintainability. so... the statement above refactored to slightly better pseudo-code: $limit = getLimitParameter(); $sqlString = SqlResourceLoader::Load("BitCoinNodeSelect.sql"); $statement = $db->prepare($sqlString); $statement->bindValue(1, $limit, PDO::PARAM_INT); $statement->execute(); ..or the query with the SQL along with parameter parsing logic could be contained in a separate class altogether.
    - emilsedgh 13y ago
      Excuse me for asking, how saving the SQL query in a file helps maintainability? Storing the query in a file means an additional IO call, right? Isn't the better approach to wrap the sql statement in a function if it's about to be reused? I'm not arguing, I'd like to know the point and learn why saving that into a file helps maintenance.
      - Nilzor 13y ago
        Scenario: You're asked to rewrite SQL for UseCase#14 because it's slow. I'd argue the developer find it more quickly if it's stored in a separate .SQL file with a file name related to the use case (discoverability). It would also separate better in source control history as you'd see when you did SQL changes and when you did PHP changes. Could be valuable when bug-hunting. edit: And the obvious: instead of scrolling to line 762, column 18 of PHP file X, you open file Y on line 1, column 1. It's also an .SQL file, which tells an IDE to syntax highlight it as SQL and not PHP. Yes, there's a performance cost. Hard disks are fast, though. And then there's caching. I always value maintanability higher than performance, as maintainability often is correlated with number of bugs. You suggest to wrap the SQL statement in a function, and that might be just as fine. There's no single best solution here. Just a lot of solutions that are better than the Bitcoin-class ;-)
      - AYBABTME 13y ago
        Hopefully the SqlResourceLoader would do some caching.
- Cthulhu_ 13y ago
  Personally, I'd extract it into its own .sql file; if your query is complex, it should be treated as code.
dschiptsov 13y ago
Yeah, especially when a synonym for MVP, stripped from all that MBA-ish pathos would be "half-backed crap". There is a classical old time running joke: "two typewriter girls talking - look, I could do 80 characters per minute, when I am in a mood. - bhaa, I could do 500.. but it is utter nonsense" In some sense it summarizes all that "from zero to market in one month with cheap coders who just getting shit done" and other MVP-related nonsense. Actually the code and how it was found speaks for it all. Worse isn't always better.
- easy_rider 13y ago
  True. Well in this case an MVP would be considered something that already processes transactions. I would think that is the minimal functionality a currency exchange would have to have. So for all reasons stated above (minus the security problems, which were not stated in above post), this could work for an MVP. (Although again I would expect the quality of the code to be initially way better in quality in regards to security ) When I start something from scratch, unlike a framework, I usually blast away in a single file, this just makes it easier for me in the prototyping phase. I will write a lot of "crappy" code, will not separate concerns but keep them close to the source for now and refactor it later. It helps me see some bigger pictures in my design, and allows for quick revision.
  - dev360 13y ago
    What language do you program in?
    - easy_rider 13y ago
      PHP, Ruby, JS.
    - easy_rider 13y ago
      The example given is PHP (and also my forte), and do have to elaborate again. This is not my style if I start an entire project from scratch. I would probably pick Symfony2 if the need would require. But I sometimes get these tasks where a client is hosted on shared host, my only option is PHP. I don't even get a shell to run console commands. I might not even have the budget to setup GIt/deployment strategies and have to work on FTP.... This means I will probably cowboy everything in to one file, and test it on there, instead of messing around with folders and files in a GUI. I've had occasions where i'd just want to open something something borked and gone were some important other production files.. Luckily in these cases you will often find improvised version control systems in place (e.g. /site_old site_old123 /site_new2 /site :D)

for an MVP this is fine :D Seeing how long mtgox has been around this is absurd. 1700+ lines would spin my head pretty fast.. And inline SQL is non-forgivable.

davidw 13y ago

> And inline SQL is non-forgivable What do PHP folk typically do for a big, hairy query that isn't really doable via an ORM? Not that this is the case here, just curious. I haven't touched PHP for years.

stingraycharles 13y ago
It's always better to use prepared statements, since it prevents SQL injection attacks. If you do not use neither an ORM or prepared statements, you either have to with trusted input, or need to sanitize that input really well (to the point it's often easier to just use prepared statements).
easy_rider 13y ago
Oops, "inline SQL" is probably a wrong way of putting it. Actually, even in Symfony2 which has an ORM (Doctrine), I sometimes find myself writing SQL queries in code because Doctrine can be so inefficient in some situations. Writing pure SQL makes your code less portable (i.e. tied to a specific database, in this case MySQL). Now I don't think its bad at all to have somewhat complex group/(sub)queries written this way if portability is not a big concern, and if the number of those queries is not as big, it is something you just need to be aware of. What we're actually talking about here which is really bad is string concatenation without parameterization. Meaning, it seems the code is not using a database driver. PDO has been the accepted way of doing this. You pass the parameters to PDO separately from the query for sanitation. We can all see why this is bad: if (isset($_GET['limit'])) { $limit = (int)$_GET['limit']; if ($limit < 1) $limit = 1; if ($limit > 10000) $limit = 10000; } $req = 'SELECT * FROM `Money_Bitcoin_Node` WHERE `Status` = \'up\' AND `Last_Checked` > DATE_SUB(NOW(), INTERVAL 6 HOUR) AND `Version` >= 31500 AND (`Last_Down` IS NULL OR `Last_Down` < DATE_SUB(NOW(), INTERVAL 2 WEEK)) AND `First_Seen` < DATE_SUB(NOW(), INTERVAL 2 WEEK) ORDER BY RAND() LIMIT '.$limit;
- Nilzor 13y ago
  Exactly. Parameterization is #1 priority. Second you could also move the statement string itself to a separate resource (disk file), which helps for maintainability. so... the statement above refactored to slightly better pseudo-code: $limit = getLimitParameter(); $sqlString = SqlResourceLoader::Load("BitCoinNodeSelect.sql"); $statement = $db->prepare($sqlString); $statement->bindValue(1, $limit, PDO::PARAM_INT); $statement->execute(); ..or the query with the SQL along with parameter parsing logic could be contained in a separate class altogether.
  - emilsedgh 13y ago
    Excuse me for asking, how saving the SQL query in a file helps maintainability? Storing the query in a file means an additional IO call, right? Isn't the better approach to wrap the sql statement in a function if it's about to be reused? I'm not arguing, I'd like to know the point and learn why saving that into a file helps maintenance.
    - Nilzor 13y ago
      Scenario: You're asked to rewrite SQL for UseCase#14 because it's slow. I'd argue the developer find it more quickly if it's stored in a separate .SQL file with a file name related to the use case (discoverability). It would also separate better in source control history as you'd see when you did SQL changes and when you did PHP changes. Could be valuable when bug-hunting. edit: And the obvious: instead of scrolling to line 762, column 18 of PHP file X, you open file Y on line 1, column 1. It's also an .SQL file, which tells an IDE to syntax highlight it as SQL and not PHP. Yes, there's a performance cost. Hard disks are fast, though. And then there's caching. I always value maintanability higher than performance, as maintainability often is correlated with number of bugs. You suggest to wrap the SQL statement in a function, and that might be just as fine. There's no single best solution here. Just a lot of solutions that are better than the Bitcoin-class ;-)
    - AYBABTME 13y ago
      Hopefully the SqlResourceLoader would do some caching.
Cthulhu_ 13y ago
Personally, I'd extract it into its own .sql file; if your query is complex, it should be treated as code.

dschiptsov 13y ago

Yeah, especially when a synonym for MVP, stripped from all that MBA-ish pathos would be "half-backed crap". There is a classical old time running joke: "two typewriter girls talking - look, I could do 80 characters per minute, when I am in a mood. - bhaa, I could do 500.. but it is utter nonsense" In some sense it summarizes all that "from zero to market in one month with cheap coders who just getting shit done" and other MVP-related nonsense. Actually the code and how it was found speaks for it all. Worse isn't always better.

easy_rider 13y ago
True. Well in this case an MVP would be considered something that already processes transactions. I would think that is the minimal functionality a currency exchange would have to have. So for all reasons stated above (minus the security problems, which were not stated in above post), this could work for an MVP. (Although again I would expect the quality of the code to be initially way better in quality in regards to security ) When I start something from scratch, unlike a framework, I usually blast away in a single file, this just makes it easier for me in the prototyping phase. I will write a lot of "crappy" code, will not separate concerns but keep them close to the source for now and refactor it later. It helps me see some bigger pictures in my design, and allows for quick revision.
- dev360 13y ago
  What language do you program in?
  - easy_rider 13y ago
    PHP, Ruby, JS.
  - easy_rider 13y ago
    The example given is PHP (and also my forte), and do have to elaborate again. This is not my style if I start an entire project from scratch. I would probably pick Symfony2 if the need would require. But I sometimes get these tasks where a client is hosted on shared host, my only option is PHP. I don't even get a shell to run console commands. I might not even have the budget to setup GIt/deployment strategies and have to work on FTP.... This means I will probably cowboy everything in to one file, and test it on there, instead of messing around with folders and files in a GUI. I've had occasions where i'd just want to open something something borked and gone were some important other production files.. Luckily in these cases you will often find improvised version control systems in place (e.g. /site_old site_old123 /site_new2 /site :D)