Django refactoring game – can you fix all the Models anti-patterns? | Hacker News Reader

Django refactoring game – can you fix all the Models anti-patterns?

django.doctor

80 pointsrikatee53 comments

This 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.

> 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?

SRP, 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.

Agree. It also makes for a sql table explosion.

Just 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?

It depends on your usage. Sometimes denormalization is what you need.

Adding unnecessary joins.

Here’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.

Besides, you can simplify the Python side of things without touching the database side of things with mixins.

"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.

This covers it quite nicely https://django.doctor/advice/C2006

Thanks 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?

I 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.

A 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.

I think what they are trying to say, is that if you have a huge max_length it is possibly because you accept any type of text, and you felt the number was high enough.

If you use postgres, you want to restrict the length (even if it is 5000) ? => CharField, you don't care about the length => TextField

I made this as a tech demo for what 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

Really neat demo. I've not used Django all that much; do you think the necessity of this tool reflects badly on its interface?

I 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

Nullable TextFields are not anti-pattern necessary. I also don't see anything wrong with explicit default arguments.

> 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.

Yes, 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.

I 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?

You 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".

You use hidden fields in the html form to preserve previous values and then compare them in your input validation code server side. You set the default value of input type text and text area to the previous value. If the value was blank before and after you save as null, otherwise you set it to the new value.

It cares very much about the difference, because it lets you set both separately.

In the real world the GitHub PR bot gives the dev the choice of committing the advice or not. I appreciate that does not reflect perfectly in the game :)

> I also don't see anything wrong with explicit default arguments.

Very much your and your team's choice. I for one see them as noise. Does it add value explicitly stating all the fields of Field? https://docs.djangoproject.com/en/3.1/ref/models/fields/#fie...

If you would not explicitly state all of the defaults why state a subset of them?

I 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

You'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 :)

I 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!

glad you liked it :)

I 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.

Very 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.

Correct answer:

  @sql
  @dataclass
  class Comment:
    body: str
    date: Date

Any db specific Metadata can be specified via fields.

Where 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...

An idea that I submitted for discussion here:

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.

Maybe, but can you refactor a model superclass that's already in use by several subclasses? You have seven days, good luck.

for that you need a human unfortunately ;)

Even 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.

I 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?

Well 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.

This 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?

I 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.

> An anti-pattern may be done for solid business reasons.

This is why linters let you ignore lines/files.

on the roadmap is config file but for now devs can just ignore the advice :)

I mean this whole game is an ad for their linter.

Please 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.

Fixed 32 out of 35, hopefully I understand django a little bit since I build our entire product on it ;)

Neat demo tho !

35?

I had 23 out of 24 issues, just a few minutes ago

If you submit the same answer twice then it adds them twice.

Future version of the quiz will close that bug :)

Cool concept (fixed 21 of 24 issues)

Lint tools are usually overly picky but this seems like a good choice of issues

Thanks! and blimey well done that's better than I did first go around

Fixed 24 out of 24 :)

Looks cool.

wow well done!

Reply on news.ycombinator.com