Backend Code Review

[f-prime]

There isn't as much to the backend as there is to the frontend, but here are a few things I noticed. There's some overlap with the frontend review.

1. Django makes it very easy to write tests, so write tests - Especially now that the codebase is so small it'd be very easy to add tests to the endpoints that you have now. I would strongly recommend taking the time (it shouldn't take long) to test your endpoints. It helps other developers out and also makes you more productive in the long run because as the codebase grows things will become harder to change without breaking something else.

2. Move URLs specific to an app into the app's folder - There aren't many routes right now so I can understand keeping everything in the root urls.py file. I recommend that if you have to add more routes, move them to a urls.py file inside of the app they are associated with. This helps keep things clean and easy to find when there are a lot of routes.

3. Better API request error handling - Specifically here: https://github.com/ezl/wodeveryday.com/blob/master/back/quickstart/views.py#L36-L37 if the API goes down or the request fails for whatever reason you'll get a 500 back. It's better to catch the error and return a message so that it's easier to debug when it happens.

4. Keep functions concise - Specifically here: https://github.com/ezl/wodeveryday.com/blob/master/back/quickstart/views.py#L72-L85 I mentioned this in the frontend review, but try to keep functions simple. You can avoid the need for the .copy() if you move this code to its own function and call it to generate the object.

5. Move out static content - Specifically here: https://github.com/ezl/wodeveryday.com/blob/master/back/quickstart/views.py#L103 This is pretty much the same example as in the frontend. I think it makes sense to move this to a variable in settings.py. It's very difficult to manage static content in code over time so move that stuff to variables whenever possible.

[ezl]

@f-prime I'm just reviewing the codebase in earnest for the first time now.

Would like your check here.

My read on this is that a lot of this is a "smell" of the data architecture choice.

For example, where you linked here: https://github.com/ezl/wodeveryday.com/blob/master/back/quickstart/views.py#L72-L85

methods like listDistinctCountriesByContinent are, IMO a consequence of the Model choice.

@cb0806151 - correct me if I'm wrong, but it looks like there's just a single model: Affiliate

There are approximately 10,000 Affiliates worldwide, each of them definitely has a Continent, Country, City, etc.

By creating models for each

class Continent(models.Model):
    models.TextField(blank=True)

# ... skipping some layers

class City(models.Model):
    country = models.ForeignKeyField(Country)
    slug = models.SlugField()
    name = models.TextField(blank=False)

class Affiliate(models.Model):
    city = models.ForeignKeyField(City)
    website = models.URLField()
    slug = models.SlugField()
    name = models.TextField(blank=False)

That allows us to basically eliminate methods like listDistinctCountriesAndTheirCitiesByContinent and listDistinctCountriesByContinent and DRF handles this stuff for us with queries like Country.objects.filter(continent=____) (not sure why distinct is an issue, but Country object instances should probably be distinct anyways, but if not, throw a distinct() on there to Country.objects.filter(continent=____).distinct().

[cb0806151]

@ezl yes, there is currently only one model/table.

I can definitely see how what you are proposing would be an improvement over what's currently in place, but I have one slight concern in that only the city stays attached to the affiliate; if I need to access the gyms continent I need to chain over through the city and country models just to get to it. However, this concern of mine seems dangerously close to pre-optimization so I'm going to ignore it; just thought I should voice it.

Should I put together a tech debt issue ticket for implementing the solution you have put together?

[ezl]

Personally, if I don't have to maintain it and iteration speed stays high, I don't care.

But I see a lot of areas in the code already where we are inventing stuff we don't have to in order to solve for problems the framework already solves for.

1. for example, the stated case above where we are creating 3 or 4 similar methods for things that the ORM already does.
2. In the client side, there's a lot of string matching and "__iexact" queries for things that are happening because we're treating .city as a string attribute of another object when they are definitely distinct things.

It's probably more of an optimization error to try to choose worse models (single flat model) for the purpose of being able to create arbitrary filters across any depth.

For me, I'm 80% focusing on just the experience (the last few days is the first time I've really looked at the code).

Overall, the project is small enough that I think we can get away with some bad choices -- and since YOU are the primary maintainer, if you're comfortable with it, I'm happy to let it ride.

[f-prime]

@ezl I noticed this too on my pass through the code but I didn't voice it because as you said there isn't much code here and if it works there's no need to fix it. That is of course unless the backend is going to be growing in which case I agree with the proposed modifications.

@cb0806151 Breaking up that affiliate model into separate models like ezl suggested would help with readability but also give you a bunch of other queries for free with much less code. ModelViewSets are incredibly powerful for these kinds of queries.

Let's take for example again the listDistinctCountriesByContinent method.

If you already have the models proposed by ezl then all you need is a ModelViewSet for Continent and modify the retrieve method to return the distinct countries in the content. You can also write ModelSerializers for all the models so that you can get the nesting. that you need.

Even if you just used APIViews, relying more on what DRF gives you through the models and serializers will make your job a whole lot easier.

When I first started using Django I did a lot of the things that you are doing. Django does A LOT of the heavy lifting for you. You can get a crazy amount of work down with very little code. It's not worth fighting the framework. ModelViewSets are your friend. If you find yourself writing a lot of code CRUD style code (this seems to be mainly a bunch of reads) try seeing if there's a Django specific solution to what you're trying to do. There often is.

If it works though I don't see any urgent reason to fix it, but continuing along this path will make it difficult to maintain in the future.