Please criticize my Spring MVC code.



  • Ok, I'm learning sql, so I decided to implement the game of life in it.

    The database I'm using is H2, which I picked at random (‘random’ here meaning that I spent less than an hour on wikipedia) out of what was available from the Spring initializer.

    setup:

    CREATE TABLE IF NOT EXISTS cells 
    (
        x   INT NOT NULL, 
        y   INT NOT NULL,  
        gen INT NOT NULL,
        UNIQUE (x, y, gen) 
    ); 
    
    
    CREATE INDEX ON cells (gen); 
    CREATE INDEX ON cells (x,y);
    
    
    DROP TABLE IF EXISTS born; 
    DROP TABLE IF EXISTS survive; 
    CREATE TABLE born ( neighbours INT PRIMARY KEY ); 
    CREATE TABLE survive ( neighbours INT PRIMARY KEY ); 
    INSERT INTO born VALUES (3); 
    INSERT INTO survive VALUES (2), (3);
    
    
    CREATE VIEW IF NOT EXISTS alive AS
     SELECT x,y 
       FROM cells 
      WHERE gen = 
            (
               SELECT COALESCE(MAX(gen),0) FROM cells
            );
    
    
    CREATE TABLE IF NOT EXISTS offsets 
    (
        x INT, 
        y INT, 
    );
    
    
    INSERT INTO offsets (x,y) 
    VALUES
       (-1, -1), (-1, 0), (-1, 1),
        (0, -1),          (0, 1),
        (1, -1), (1, 0), (1, 1);
    
    
    CREATE VIEW IF NOT EXISTS next AS 
    SELECT 
           x, 
           y, 
           n IN (SELECT neighbours FROM born) AS can_create,
           n IN (SELECT neighbours FROM survive) AS can_survive 
      FROM 
           ( 
             SELECT 
                    alive.x+offsets.x AS x, 
                    alive.y+offsets.y AS y, 
                    count(*) AS n 
               FROM alive 
               JOIN offsets 
           GROUP BY x,y 
           )
     WHERE 
           n IN (SELECT neighbours FROM born)
           OR 
           n IN (SELECT neighbours FROM survive);
    

    Update:

    INSERT INTO cells (x,y,gen)
          SELECT 
                 next.x, 
                 next.y, 
                 ?
            FROM 
                 next 
       LEFT JOIN 
                 alive 
              ON 
                 next.x = alive.x 
                 AND 
                 next.y = alive.y
           WHERE 
                 alive.x IS NOT NULL AND next.can_survive 
                 OR 
                 alive.x IS NULL AND next.can_create;
    

    ? is SELECT MAX(gen) FROM cells + 1

    1. It feels weird not to have a primary key on anything. Should I do that?
    2. I worry about the index on (x,y), especially since H2 doesn't support materialized views: is there a way to tell the db to build indexes independently for each generation?
    • I could create a separate table for each one?
    1. Is there a better way to do the join on offsets?
    2. Finding the current generation by taking the max of cells.gen is a bit weird: is there a better way to keep track of a single variable?
    • I suppose it would be nice if I could do the update without inserting a param.
    1. Anything else?
    2. (Edit) How to ensure no cells are added except to the current generation?

  • FoxDev

    Why are you writing the Game of Life in SQL of all things? There's gotta be better things to do to learn it 😕



  • Well, my ‘vision’ is to have a persistent, infinite canvas, where you could say place some cells at 10^100 or whatever, and be able to look up any position on the board at any number of generations ago. I'm not sure where else to store that information if not in a database.

    Tangentially: do you know what my options are for panning and zooming across an infinite grid in html?


  • FoxDev

    @Buddy said:

    Tangentially: do you know what my options are for panning and zooming across an infinite grid in html?

    Short of using a similar technique to Discourse's infiniscroll, I can't think of anything…



  • Figures. Though at least it could be smoother than infiniscroll, because the cells are all the same size.


  • ♿ (Parody)

    @Buddy said:

    I'm not sure where else to store that information if not in a database.

    Store, yes. Maybe answer interesting questions about particular "games." But implementing, no. Now you're putting your business logic into the DB. So you need some triggers, if I read other threads right.

    @Buddy said:

    It feels weird not to have a primary key on anything. Should I do that?

    Yes. You defined a unique index. Just make that the PK.

    I'm not really following the logic behind born and survive and your view doesn't seem to care about generations. I need some breakfast, so I'm not going to try to expend more energy on this right now.


  • Discourse touched me in a no-no place

    @boomzilla said:

    your view doesn't seem to care about generations

    That's a critical problem.



  • @boomzilla said:

    implementing, no

    I suppose we could put this somewhere between “I know it's bad, but I'm doing it anyway” and some kind of trendy, buzzword, “move the code to where the data is” stuff that I suspect someone I'm trying to get a job from would appreciate.

    I more-or-less already understand what normalization is and why it's important; my goal for this exercise is to see if I can write efficient queries.

    @boomzilla said:

    I'm not really following the logic behind born and survive

    http://en.wikipedia.org/wiki/Conway's_Game_of_Life#Variations_on_Life

    I suppose it's not strictly necessary to put those into separate variables since I'm probably never going to change them, I just had the idea to do that and then I did it.

    @boomzilla said:

    your view doesn't seem to care about generations

    I've got two views: alive that selects from cells where gen = max(gen), (with a redundant coalesce because I'm a dummy) and next that queries alive. I realize that max gen won't advance when there are no cells in the next generation, but that doesn't seem like a bug that's worth fixing.


  • ♿ (Parody)

    @Buddy said:

    I suppose we could put this somewhere between “I know it's bad, but I'm doing it anyway” and some kind of trendy, buzzword, “move the code to where the data is” stuff that I suspect someone I'm trying to get a job from would appreciate.

    Not that it doesn't sound like a fun sort of thing. But just keep in mind that you're abusing stuff, so what you learn is going to combine the useful with the harmful.

    @Buddy said:

    I've got two views:

    Ah, OK. My pre-breakfast self didn't follow the rabbit hole far enough to comprehend.


  • Discourse touched me in a no-no place

    @Buddy said:

    I've got two views: alive that selects from cells where gen = max(gen), (with a redundant coalesce because I'm a dummy) and next that queries alive. I realize that max gen won't advance when there are no cells in the next generation, but that doesn't seem like a bug that's worth fixing.

    OK, that ought to work provided you've got the right transaction isolation. It might be easier to get alive to also report the generation value so that you can easily use that plus one in your INSERT.



  • @Buddy said:

    Ok, I'm learning sql, so I decided to implement the game of life in it.

    I have a criticism right away.

    ... why!?

    You do understand that SQL is a query language, right? That's what "QL" stands for.

    @Buddy said:

    The database I'm using is H2,

    Never even heard of it. Critique #2: use products there's a small chance someone else in the universe is also using.



  • @Buddy said:

    I more-or-less already understand what normalization is and why it's important; my goal for this exercise is to see if I can write efficient queries.

    But you've chosen such a weird application, anything you learn here won't be applicable anywhere else. In fact, any optimization that works here would probably be detrimental to a real-world usage of SQL.


  • ♿ (Parody)

    @blakeyrat said:

    ... why!?

    :rolleyes:

    I knew we'd get this shit.

    @blakeyrat said:

    Never even heard of it.

    But you've heard of "Shmorky," so I don't see where your appeal to authority gets us.

    @blakeyrat said:

    In fact, any optimization that works here would probably be detrimental to a real-world usage of SQL.

    I'm not convinced of that. Still, this is largely congruent with my "you're learning to abuse SQL" warning.



  • @boomzilla said:

    I knew we'd get this shit.

    Yeah, well, if he had explained why in the first post, I wouldn't have had to type that easily-anticipated response.


  • ♿ (Parody)

    No, you'd have typed your alternative obvious criticism of what people think is fun. Or you could have offered a less vehement opinion on why GoL is not a good vehicle for learning SQL.


  • ♿ (Parody)

    I mean, this is coding help. And abuse isn't helping, or so I've been told by other people requesting help here.



  • This post is deleted!


  • how much time are you willing to invest?

    i'd suggest you go and learn the relational model and then sql. it's easier that way.
    also, normalisation and denormalisation.

    link dumped randomly:



  • @dkf said:

    OK, that ought to work provided you've got the right transaction isolation.

    Well, that statement should be the only one changing the generation, so I would hope that it would be safe enough. Or is it really possible for the result of selecting from a table to change as a result of inserting to it in the same statement?

    The other thing is ensuring that when somebody adds cells, they only add them to the latest generation, I guess there could be a trigger or something that rejects insertions if the given generation doesn't equal the current generation, but that still leaves the question of what happens if the generation update occurs during a batch cell-creation job. I suppose I really do need to learn how transactions work.

    @dkf said:

    It might be easier to get alive to also report the generation value so that you can easily use that plus one in your INSERT.

    Yeah, that makes sense, it just feels a bit weird selecting a a whole bunch of copies of the same value.

    @blakeyrat said:

    ... why!?

    Well, I guess it seemed like this would be easier to focus on than domain modeling, as in, there's more query writing involved in this per unit of work. It's hard to think of any other problem that would involve custom queries, that I could produce a simple app for.

    @blakeyrat said:

    Never even heard of it.

    Fair enough. Most of the info I got while figuring out how to do what I wanted did relate to other databases though, and it didn't seem that hard to translate.

    @blakeyrat said:

    anything you learn here won't be applicable anywhere else.

    Except, again, even though this problem is a toy problem, most of the actual learning came from reading about real-life problems, that I found in the course of trying to solve this one.

    @boomzilla said:

    I mean, this is coding help. And abuse isn't helping

    On the other hand, I did ask to be criticized, though I suppose I would have preferred to have my indentation style nitpicked, or to find out if alive.x IS NOT NULL is a valid idiom or if I would have been better off doing a union.

    @Jarry said:

    i'd suggest you go and learn the relational model and then sql. it's easier that way.also, normalisation and denormalisation

    I have read those wiki articles, but I haven't yet found a good reason (as in: a project) to put them into practise. I've thought about doing a codesod pointing out some of the normalization fails I saw in the discourse code, the only one of which I can remember being that replies are related by post_number_in_thread, instead of by primary key, which just seems so insane I can hardly comprehend it. Like, now they have to do post_number = ? AND thread_id = thread_id on every query for it, and for what? Just to break cross-thread replies? Why would that even be desirable?


  • ♿ (Parody)

    @Buddy said:

    I have read those wiki articles, but I haven't yet found a good reason (as in: a project) to put them into practise.

    This is often my biggest barrier to learning things. I don't always pick up concepts without doing them and I find it hard to stay focused without needing to or a hook to make it fun.


  • Java Dev

    @Buddy said:

    I worry about the index on (x,y), especially since H2 doesn't support materialized views: is there a way to tell the db to build indexes independently for each generation?

    I don't see how a materialized view would help here. On oracle I'd recommend partitioning on gen and using a local index; this would function similar as an index on (gen, x, y) except the continually-increasing gen won't unbalance the index.



  • The letters are small.



  • @Buddy said:

    I'm not sure where else to store that information if not in a database.

    Screenshots.



  • @Buddy said:

    Ok, I'm learning sql, so I decided to implement the game of life in it.

    How the hell are you implementing a board game using just SQL?

    @Buddy said:

    cells

    Oh, that game of life.

    Still SQL seems a bit :wtf: as a game platform.



  • @abarker said:

    Still SQL seems a bit as a game platform.

    You'd be surprised at what some people have done.



  • @Groaner said:

    You'd be surprised at what some people have done.

    No, I probably wouldn't. I use Discourse, after all.


  • Grade A Premium Asshole

    @abarker said:

    No, I probably wouldn't. I use Discourse, after all.

    Would you be surprised to find out that someone implemented the game of life...in the game of life?

    Life in life – 01:30
    — Phillip Bradbury


  • Grade A Premium Asshole

    @boomzilla said:

    This is often my biggest barrier to learning things. I don't always pick up concepts without doing them and I find it hard to stay focused without needing to or a hook to make it fun.

    Same here. I can read and go over theory until my eyes bleed. I can talk it to death. I can go over tutorials over and over.But I learn nothing but jargon until I scratch my own itch.



  • @Polygeekery said:

    @boomzilla said:
    This is often my biggest barrier to learning things. I don't always pick up concepts without doing them and I find it hard to stay focused without needing to or a hook to make it fun.

    Same here. I can read and go over theory until my eyes bleed. I can talk it to death. I can go over tutorials over and over.But I learn nothing but jargon until I scratch my own itch.

    +1
    Inversely, for me, when I do actually use something it doesn't take particularly long to learn it.



  • If you want to learn SQL, this is not the ideal use case. Better make a TODO list app or a blog or something like that. It will give you a better insight into the kind of issues you'll experience in "real world".

    If you want to screw around with a Conway's game implementation with infinite history, I'd look into some specialized kind of db that's better suited for this. For example, couchdb, that preserves full change history for each document. Or even better, a graph db that has a concept of node interconnection. Eg. neo4j with some kind of versioning plugin.

    Otherwise... I don't have major complaints. Even for no primary key thing. You're obviously going off the beaten path, so there's no purpose discussing best practices.

    You could keep the current generation (max gen query) in a separate table, just to ensure this scales.

    You can't ensure only the current gen is added in SQL code. The only thing that comes to mind is to have a "current gen" table and "history" table. But you would still need code to make sure the transfer is done correctly. In postgres, I would use stored function for generating "next step". Not sure if your db supports stored functions and/or procedures, though.


  • FoxDev

    wait... why did that stop there?!

    they were about to pill inception and show The Game of life coded in the game of life coded in the game of life!


  • kills Dumbledore

    @cartman82 said:

    You can't ensure only the current gen is added in SQL code

    You could with a trigger



  • @Jaloopa said:

    You could with a trigger

    You would need a select inside a trigger. Very iffy.



  • I don't like the extra spacing in between the parens on some lines.
    I also don't like the use of all-caps keywords.
    I don't like the fact that you don't have a space after some of the commas (but some others you do, wtf?!).
    I don't like the extra comma in CREATE TABLE IF NOT EXISTS offsets after y INT.
    I also don't like the formatting around the values getting inserted into offsets.
    The indentation on the CREATE VIEW... and INSERT INTO cells.. is attrocious.

    But other than that, whatever, it's alright I guess.



  • Here's a flamewar I can really get into.
    @Bort said:

    I also don't like the use of all-caps keywords.

    DIAF.


  • Java Dev

    I don't mind some caps - typically the style used here, with keywords in caps and tables/columns in lowercase, makes it easy to read (I work with SQL embedded in other languages a lot, where you don't get syntax highlighting).

    Our corporate coding style requires all unquoted words to be in uppercase, even in .sql files. This tends to be over the top and shouty. We ignore it.



  • @PleegWat said:

    I don't mind some caps - typically the style used here, with keywords in caps and tables/columns in lowercase

    You shouldn't enforce capitalization rules on references to object names. If a table is defined as "Customers", refer to it as Customers, not customers or CUSTOMERS. In some contexts it will matter.

    Ignore what I said above if you use Oracle. This actually doesn't work on Oracle:

    CREATE TABLE Customers (thisisacolumn int);
    
    SELECT * FROM "Customers"
    

    ...but this does...

    CREATE TABLE Customers (thisisacolumn int);
    
    SELECT * FROM "CUSTOMERS"
    

  • Java Dev

    @Jaime said:

    Ignore what I said above if you use Oracle.

    This.

    Anyway, 'always use caps' applies to definitions as well.



  • @PleegWat said:

    Anyway, 'always use caps' applies to definitions as well.

    In reality, it applies only to definitions. If your rule is that all objects are defined with uppercase names, but all object references are exactly as defined, then you will get uppercase references. No need to force references to uppercase when it sometimes causes problems.

    If you really want to mess with someone using Oracle, do this:

    CREATE TABLE "Customers" (thisisacolumn int);
    
    SELECT * FROM Customers
    SELECT * FROM CUSTOMERS
    SELECT * FROM "Customers"
    

    Only the third SELECT will work.



  • Does anyone actually know the semantics of the @Transactional annotation? I want my cell update method to take int generation, check that generation matches the current gen, add a bunch of rows, then check the generation again (double checked locking lol) and roll back the transaction if the generation has changed.

    Also, I would like to be able to call that method from a controller: if I've got the bean with that method on autowired in the controller, will the annotation apply if I call the method directly, or do I somehow need to invoke the Spring runner?


  • Discourse touched me in a no-no place

    @Buddy said:

    Does anyone actually know the semantics of the @Transactional annotation? I want my cell update method to take int generation, check that generation matches the current gen, add a bunch of rows, then check the generation again (double checked locking lol) and roll back the transaction if the generation has changed.

    The semantics will depend on what isolation level you select (and a bunch of other things that probably don't matter). You can probably start out with the SERIALIZABLE isolation level, which will ensure that no change can happen while you're changing things. (That might be too strict; REPEATABLE_READ may be enough.) That means that you can completely ignore the fact that you're making changes in your code to do the changes, which makes things easy. 😄

    @Buddy said:

    Also, I would like to be able to call that method from a controller: if I've got the bean with that method on autowired in the controller, will the annotation apply if I call the method directly, or do I somehow need to invoke the Spring runner?

    The transactional annotation's magic is applied at the Spring bean level under normal circumstances; you'll have to make sure you call via a bean and not some other mechanism (such as via this). This is because beans are really proxies of various kinds, and that's the place where the injected transaction management aspect does its magic. Even knowing what's really going on under the covers, I think it's very clever stuff and works really well.



  • @dkf said:

    That means that you can completely ignore the fact that you're making changes in your code to do the changes, which makes things easy.

    Excellent!

    @dkf said:

    you'll have to make sure you call via a bean and not some other mechanism (such as via this)

    Ok, so for this

    @RestController
    @RequestMapping("/cells")
    public class CellController {
        @Autowired Game game;
    
        /* ... */
        
        @RequestMapping(method={RequestMethod.POST})
        public boolean setCells(@ModelAttribute List<life.models.CellModel> cells, int generation) {
            return game.setCells(cells, generation);
        }
    
        /* ... */
    }
    

    I should annotate CellController.setCellswith @transactional too? Or can I call game.setCells via spring somehow? Specifically, what is the best practise for returning error from the API here?



  • @dkf said:

    That means that you can completely ignore the fact that you're making changes in your code to do the changes, which makes things easy.

    Excellent!

    @dkf said:

    you'll have to make sure you call via a bean and not some other mechanism (such as via this)

    Ok, so for this

    @RestController
    @RequestMapping("/board")
    public class CellController {
        @Autowired Game game;
    
        /* ... */
        
        @RequestMapping(method={RequestMethod.POST})
        public boolean setCells(@ModelAttribute List<life.models.CellModel> cells, int generation) {
            return game.setCells(cells, generation);
        }
    
        /* ... */
    }
    

    I should annotate CellController.setCellswith @transactional too? Or can I call game.setCells via spring somehow? Specifically, what is the best practise for returning error from the API here?



  • Ok, and for the html, I've realized that I don't actually need to change the dom unless the windows size changes: I can simulate panning by having a fixed size grid two columns wider and taller than the viewport and aliasing.

    This raises the question: should I add dom elements where the live cells are? Semantically, this feels like the correct option, but on the other hand, if I can avoid triggering a reflow, it might be worth it to show where the cells are with a $(td).css({"background-color": liveColor});.
    Back on the first hand, I suppose I would be triggering a reflow by changing the table's left and top properties anyway, should I just do the correct thing and not worry about performance?
    Ok, never mind, looks like I've answered my own question.


  • Discourse touched me in a no-no place

    @Buddy said:

    I should annotate CellController.setCellswith @transactional too? Or can I call game.setCells via spring somehow? Specifically, what is the best practise for returning error from the API here?

    Wherever makes sense, which really depends on what else is going on. The main thing is that you should make it commit by returning normally through the transaction (trailing) boundary, and rollback by throwing an exception through the transaction boundary. For a method this simple, it's not really all that important whether you put it on the controller or the model (Game) but it matters a lot when your controller needs to do several things within a transaction. In fact, if you were doing complicated stuff (which you don't appear to be) then I'd suggest putting annotations in both places: the model would have the transaction propagation on its methods set to MANDATORY (so callers need to sort the transaction out before calling) and the controller would have transaction propagation set to REQUIRES_NEW (i.e., force a transaction to be started there) but you'd need to be doing rather more complex business logic than you've shown so far for that to be justifiable.

    As long as you — or Spring's code — call via the bean (which is a wrapper round the instance of the class you wrote) then it should work. Or fail if you've left something important undone, such as not actually binding a DB context. 😄 I suggest from experience that you take time to test carefully.

    (I prefer to do my REST interfaces a bit differently, using Apache CXF, but that's a personal choice and not a criticism of you at all. My interfaces are really quite complicated! Don't change because I'm using something different to you.)


  • Discourse touched me in a no-no place

    @Buddy said:

    Ok, never mind, looks like I've answered my own question.

    Hehe.

    I'd actually suggest looking at using an HTML5 <canvas> to do the rendering. The performance is reasonable, and you don't have to worry about mucking around with changing the DOM. The downside is that you must do lots of JS in the client then. Still, you're a competent programmer so it shouldn't be too hard; for graphical work, it beats using the DOM massively. There are reasonable tutorials online; I got something going within a couple of weeks a few years ago, and that was the first JS project I ever did. ;-)



  • Yeah, that's true, render graphics in the graphics element. Although, as with the abuse of SQL, getting familiar with the DOM is probably more important for me right now than making this particular project good or efficient.



  • Ok, it's all starting to make sense now. Thanks for the help!


Log in to reply