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;
?
isSELECT MAX(gen) FROM cells
+ 1- It feels weird not to have a primary key on anything. Should I do that?
- 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?
- Is there a better way to do the join on offsets?
- 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.
- Anything else?
- (Edit) How to ensure no cells are added except to the current generation?
-
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?
-
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.
-
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.
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
andsurvive
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.
-
-
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.
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.
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) andnext
that queriesalive
. 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.
-
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.
I've got two views:
Ah, OK. My pre-breakfast self didn't follow the rabbit hole far enough to comprehend.
-
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 yourINSERT
.
-
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.
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.
-
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.
-
... why!?
I knew we'd get this shit.
Never even heard of it.
But you've heard of "Shmorky," so I don't see where your appeal to authority gets us.
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.
-
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.
-
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.
-
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:
-
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.
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.... 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.
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.
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.
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.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?
-
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.
-
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.
-
-
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?
cells
Oh, that game of life.
Still SQL seems a bit as a game platform.
-
Still SQL seems a bit as a game platform.
-
You'd be surprised at what some people have done.
No, I probably wouldn't. I use Discourse, after all.
-
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?
-
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.
-
@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.
-
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!
-
-
-
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 inCREATE TABLE IF NOT EXISTS offsets
aftery INT
.
I also don't like the formatting around the values getting inserted intooffsets
.
The indentation on theCREATE VIEW...
andINSERT 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.
-
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.
-
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"
-
Ignore what I said above if you use Oracle.
This.
Anyway, 'always use caps' applies to definitions as well.
-
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 takeint 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?
-
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.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.
-
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!
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.setCells
with @transactional too? Or can I callgame.setCells
via spring somehow? Specifically, what is the best practise for returning error from the API here?
-
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!
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.setCells
with @transactional too? Or can I callgame.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.
-
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 toMANDATORY
(so callers need to sort the transaction out before calling) and the controller would have transaction propagation set toREQUIRES_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.)
-
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!