12 ms·
Django refactoring game – can you fix all the Models anti-patterns?
- rikatee 6y agoI made this as a tech demo for what https://django.doctor https://django.doctor is capable of doing in Pull Requests when it reviews your code. Django Doctor is a code improvement bot you can add to GitHub to review your PRs
- rozab 6y agoReally neat demo. I've not used Django all that much; do you think the necessity of this tool reflects badly on its interface?
- rikatee 6y agoI think it's a good thing. It indicates: - Django is big (because it solves a lot of problems) - there exists a learning curve Sometimes people miss best practices when the interplay between those two points occur. Fortunately bots exist to help with that problem
- deleted 6y ago[deleted]
- raverbashing 6y agoCool concept (fixed 21 of 24 issues) Lint tools are usually overly picky but this seems like a good choice of issues
- rikatee 6y agoThanks! and blimey well done that's better than I did first go around
- golergka 6y agoI haven't touched Django in the last 6-8 years, but I've been able to solve first few problems just with basic common sense and SQL knowledge. Are those problems really about Django the framework and not about data modeling in relational databases?
- rikatee 6y agoWell done. Shows either you're a very intuitive dev, or Django is a very intuitive tool. Maybe both. These problems I think are more to do with inertia and not having best practices in mind. It's easy to do CharField and not thing about TextField. Plus over time requirements change, so when a model was created maybe the kwargs were perfectly valid - but over time as refactors occurred tech debt crept in.
- golergka 6y agoThis makes me curious; wouldn't it be possible to write an automated linter or static code analyzer that would catch and automatically fix these issues? In Node environment, I think that I would be able to come up with an ESLint plugin. Doesn't Python ecosystem have something similar?
- rikatee 6y agoI think not because a think the bot cannot know is "why". An anti-pattern may be done for solid business reasons. It's Django Doctor's job to say "x could be y" and it's the dev's job to approve/reject the advice based on what the dev knows about the business and problems being solved.
- danielvinson 6y ago> An anti-pattern may be done for solid business reasons. This is why linters let you ignore lines/files.
- rikatee 6y ago
- fleetfox 6y agoNullable TextFields are not anti-pattern necessary. I also don't see anything wrong with explicit default arguments.
- Alex3917 6y ago> I also don't see anything wrong with explicit default arguments. I think explicit default arguments should be used when they're documenting some behavior that the reader might not know or remember, but that isn't the case here. E.g. with python's enumerate built-in, it's useful to the reader to know that it starts at 0 or whatever. But with Django's models, adding (null=False, blank=False, editable=True) isn't telling the reader anything that they likely wouldn't already know or assume, it's just making the code more difficult to read.
- japhyr 6y agoYes, I first read those as (null=True, blank=True) because I've never written out those arguments when their default is False. So for me writing them out explicitly with their defaults is an anti-pattern.
- JimDabell 6y agoI really hate how Django doesn’t care about the difference between null (the lack of a value) and the empty string (the presence of a specific value). But if you’re going to use HTML forms as a way of editing that data in a generic way, I don’t really see a better option. How would you present a form containing a text field that was null in such a way that it would be preserved after the user edits a different field and saves?
- pmontra 6y agoYou should probably check the data coming from the form and what's in the database. If the form sends an empty string for a null field in the database, you keep the null. If it sends an empty string for a not null field, you set an empty string in the db field. This assumes that the semantic of null is "never touched before".
- glenaddict 6y agoI do not like that you expect redundant fields be omitted. Just because they might be the default does not mean you shouldn't show your business rules
- rikatee 6y agoYou're right - that's why the GitHub bot just gives "food for thought" advice and does not block the merge. It's "have you considered" rather than "you must do this". The dev has the choice of if to commit the change that is suggested That gets a bit lost in the game I know :)
- Syntonicles 6y agoI have worked on similar projects to this game in the past (at scale) so it's close to my heart. To me the benefit of the default redundant arguments error is pedagogical. It forces me to think more closely about what it means to set nullable, assuming I was coming to the game with a limited understanding of Django code. Just glimpsing a note in the docs doesn't register nearly as quickly interactive play does. I would love to see this format used more extensively. When I'm learning an API I tend to fire up a REPL and explore it by poking. I do it even with basic mathematics that have fallen out of my brain. Very much enjoyed and it served as a quick refresher!
- rikatee 6y agoglad you liked it :)
- georgyo 6y agoI think for code readability show defaults can be confusing, unless you _always_ show those defaults. The following implies that bar might be null-able if you don't know what the default is. foo = text(null=False) bar = text() While writing code you likely have the defaults you care about in your head. Someone reading your code may not. You want to make sure your code leads their thought process down the right path. So either you _always_ show the default, or you only override the default.
- adsharma 6y agoCorrect answer: @sql @dataclass class Comment: body: str date: Date Any db specific Metadata can be specified via fields.
- blackrobot 6y agoWhere does the `@sql` decorator come from? If it's not adding attributes that are django specific, then you lose most of django's functionality built around models: automatic admin, user model, etc...
- adsharma 6y agoAn idea that I submitted for discussion here: https://github.com/ppinard/dataclasses-sql/issues/4 https://github.com/ppinard/dataclasses-sql/issues/4 My belief is that the django functionality can be added back in the form of decorators. I have similar work to add @graphql to python dataclasses.
- tyilo 6y agoFixed 24 out of 24 :) Looks cool.
- rikatee 6y agowow well done!
- polote 6y ago> A taller Model with many fields [...] It may be time to split the Model into multiple Models with a OneToOne relationship. Never understood that advice, what's the point of complexifying your model because a class has too many attributes?
- jrvarela56 6y agoSRP, makes it easier to test a unit if you know there's a specific/small dependency between the big model and the new small model. Also makes it easier to understand by giving a set of fields a name, sort of like a summary.
- tarkin2 6y agoAgree. It also makes for a sql table explosion.
- mushishi 6y agoJust a thought when it might be viable: if a subset of that table receives a lot of updates, and it would be helpful to your access patterns if some of it was separated into a different table that was rarely updated but accessed a lot?
- vangelis 6y agoIt depends on your usage. Sometimes denormalization is what you need.
- nitely 6y agoAdding unnecessary joins.
- GauntletWizard 6y agoHere’s a good example of 1:1 denormalization: Say I’m running a music streaming service. I can store my music tracks as a giant database of MP3s, ID3 tags included, and parse out each ID3 tag on each read. I could, alternatively, store the music tracks as a giant database of music tracks, which has “Artist”, “Album”, etc. fields, and one field for “MP3” that contains no id3 tags. Both are “normalized” data structures. Storing the id3 tags with the MP3 would be denormalization. It might be much saner to do that than to reconstruct them every time someone goes to download them. For performance, though - You want to store the MP3s in their own table, or not even in a “table” but as a blobstore. Storing the MP3 files for your tracks inline is entirely feasible. 3-10MB inline blobs will work in your modern database engine of choice, and simplify a lot - But you’ll often want to write your presentation layer with “SELECT * FROM tracks” - And you can do that if you just store the data blob separately; i.e. denormalize. If you have hundreds of fields on a record, selecting from the subset that are actually relevant is far, far saner. Particularly when a large group can be nullable, and will likely be nullable together. If 10% of your business is washers - Washers of every shape, thickness, hole-size, material, etc. - You will have all of those attributes in your parts database. It’s best to treat those as first-class citizens, but they belong in a nullable 1:1 relationship.
- qwerty456127 6y agoVery fun. This is the form of studying I enjoy the most - by correcting "other's mistakes" (and getting hints when I can't). I actually believe it is very efficient. I wish there were more games like this, for all the subjects imaginable.
- solox3 6y ago"CharField with huge max_length (5000)" If I'm on postgres and actually want my comments to have a max length of 5000, is it suggesting that using CharField(max_length=5000) is a problem with my design? Using CharField / VARCHAR vs TextFIeld / TEXT should not have a performance difference in postgresql.
- rikatee 6y agoThis covers it quite nicely https://django.doctor/advice/C2006 https://django.doctor/advice/C2006
- solox3 6y agoThanks for replying, and yes that is the page about which I have questions. * The error code C2006 assumes there is a clear divide between "smaller" and "larger" strings. What is smaller, and what is larger? Is 1000 large? Is 5000 large? Is 1000000 large? If I want my comment to be capped at 5000 characters to encourage healthy discussions but prevent spamming, is it large? * If a field has no "known format" and that's where it draws the line between CharField and TextField, then why is the error C2006 still raised when TextField(max_length=5000) is used? Why does the validator recommend allowing anyone to insert entries of unlimited size?
- nickjj 6y agoI don't know about Django's ORM but normally I would use a TEXT field and then set a separate char_length CONSTRAINT on the field. I think it's important to have the length constraint at the database level for data consistency's sake. Then you can choose to add in app level validations to make more friendly user error messages. Changing a constraint's length is also a much easier database operation vs. changing a varchar's length.
- emptysea 6y agoA nice advantage of having a TextField with a check constraint, instead of a varchar, is that the locks required in Postgres to change the check constraint are less extreme than the ones required to change the length of varchar. With a check constraint, we can use `NOT VALID` with a separate `VALIDATE` call, while a change to the varchar length requires an `ACCESS EXCLUSIVE` lock.
- TheRealPomax 6y agoMaybe, but can you refactor a model superclass that's already in use by several subclasses? You have seven days, good luck.
- rikatee 6y agofor that you need a human unfortunately ;)
- TheRealPomax 6y agoEven though you shouldn't, because the only thing the human needs to do is basically trivial. It's genuinely mind-boggling why, after 15 years, Django still suffers from incompletions like this.
- gingahbread31 6y agoFixed 32 out of 35, hopefully I understand django a little bit since I build our entire product on it ;) Neat demo tho !
- dang 6y agoThis project had a Show HN just a couple weeks ago, so I've taken "Show HN" out of the title. Please see the rules: https://news.ycombinator.com/showhn.html https://news.ycombinator.com/showhn.html.
- ahmedfromtunis 6y agoPlease make a (fully fledged) out of this. As a Django user (and big fan), I enjoyed playing this little game and would love it too spend much time on it. Plus if would be a great way to learn new stuff.