search

Cabal-Syndicate / randora_website

Cabal Syndicate's website
1 stars 2 forks source link

Users App Code Review #13

Open Master-Foo opened 9 years ago

Master-Foo commented 9 years ago

OK, I took some time to review the Users App. I saw nothing which would be considered "inefficient"

That's not to say there isn't any room for improvement though.

Class Based Views

First, let's talk about "Class Based Views" Multi-Page Chapter Here. I didn't want to bring this up just yet, because it's kind of an Advanced Topic and I didn't want to burden you with it just yet. Also, it's not required yet. Also, it's an easy re-factor for when the time comes.

But, here it is anyway. Don't worry about this info dump too much. Just read it and mull it over in your head.

  1. In Django, "Controllers" are actually called "Views" I might occasionally mix these up. So, note that, when I say "Controller" (in the traditional MVC sense of the word), I'm actually referring to what is a "View" in Django. Djano calls the traditional "View" (in the MVC sense of the word) a "Template". I'll try to stick with Django terminology.
  2. The tutorial has, thus far, used simple functions for each "View" / "Page", really what a view is, is an "Action" which a client can access. So, typically they will be named by verb, because a verb is something you do. like "login", "logout", etc. You've mostly stuck with this, (With the exception of "profile", which, we'll get to later...) This is mainly FYI.
  3. The main reason we use Class Based Views instead of simple functions is:
    1. It makes "RESTful" develpoment easier.
    2. It's easier to keep "DRY."
    3. It encourages "Everything in it's place."
    4. It encourages "Do one thing and do it well."
    5. We can use it to morph our data into different doc types. (HTML, JSON, XML, Spreadsheet, etc.) Using the same code base.
    6. We can use it to modularize our Views such that we can insert related "Message Board" template into, for instance, the "User's" template using RESTful AJAX calls.
    7. We can also move each "Action" into it's own file. A lot of the reason the User's View is so intimidating is because it spans more than one screen in a text editor. If you were to look at my code base in the Framework, you will notice ALMOST NOTHING is more than one screen of text. This makes everything easier to understand.
    8. It is easier to Unit Test.
    9. We can lock down what individuals can do using permissions.

So, the way to move forward with this is to create a "views" folder inside the "Users" App. Then you'd convert the functions you have into "Class Based Views".

In other words, you'd have a file for login.py, logout.py, register.py, etc.

In a "Class Based View" there are built-in functions for handling HTTP methods. So, you'd specify that you want to do certain tasks when POSTing and other tasks when GETing.

For instance, if you are processing submitted "register" or "login" data, you'd do this in the POST method. If you want to display a form for these actions, you'd specify that in the GET method.

Now, getting back to the issue of the "profile". Really what this would be is a GET method inside the Users "index" view. Oh, BTW, "index" isn't a verb, I admit, sometimes there are exceptions to the rule. But generally, we don't literally specify "index" in the URL, for instance:

http://blah.com/users/master_foo/index (This would take us to the profile)

vs.

http://blah.com/users/master_foo/ (This would also take us to the profile, but we didn't specify this explicitly)

So, there is a "Good Reason" to violate the rule in this instance of naming "index" instead of a verb.

Templates

Nothing too big here. One thing of note though. In users_base.html You have an <html> tag. I'm not sure if this is on purpose or a mistake. But I'll just give you the general idea of how templates "should" work.

The website's base app (In this case the Randora app) should be taking care of all "site wide relevant" GUI stuff. So, you'd have a template which includes the site header, footer, etc. and in that template there would be code that says "Insert module code here". and it will insert the template for "User", "Message Board etc." into that template.

Meanwhile, the User template will only have the html relevant to what you are doing for User. So, there should never be an html tag or meta tag, etc. in App templates.

URLS

This is OK for now, we'll have to change it a bit to conform to Class Based Views, but this is trivial.

Models

Can't say much about this. There is nothing there. I'm assuming you are working to extend this, as it is a requirement of mapping relationally to other Apps, I will say this though. Your model will be huge, as this is the workhorse of the App. The general mantra in MVC is "Fat Model, Skinny Controller" which means, most of your work should be in the Model, not the controller.

Tests

As explained earlier, don't worry about this yet. Until we are done with the prototype, we'd just be chasing windmills. (Rewriting unit tests as the spec changes).

Forms

This looks good for now. Eventually, we will be auto generating forms from the model. But we aren't there yet. Just keep this in mind.

Migrations

Looks good. Are you creating these manually, or using ./manage.py? It doesn't matter which, I'm just wondering.

Fixtures

Even if you aren't writing Unit Tests just yet, do write some fixtures to populate the database with dummy data. This way I have some test users, test messages, etc. When I review your progress.

More info Here

That's kind of a large info dump. Nothing here is something that needs to be taken care of NOW. So, just be aware of it and think it over, so it won't be a surprise in the future.

FFX01 commented 9 years ago

Thanks for all the information. I'll read up on the class based views and start trying to implement them.

As far as the migrations go, I have been using the manage.py makemigrations, migrate method. Syncdb is deprecated as of the next version, so i figure i should start using the new method. How would I do the migrations manually if I had to? Using an SQL script?

Master-Foo commented 9 years ago

Migrations are just a file in the migrations folder. You can read one to see what is going on. There is no SQL.

FFX01 commented 9 years ago

OK, I read my migration files. I see what's happening. I'm going to build a sample project implementing some of the concepts that you've introduced here so I can get some experience with them. I will then apply this experience to the Randora website project. Shouldn't take me long, as I'll be working on it all night tonight. I'm having a lot of fun learning all this stuff.

It's interesting how much knowledge is required to work within a framework. It's also interesting how much conceptual knowledge you gain about how a server operates regardless of which framework you're using.

Master-Foo commented 9 years ago

It's interesting how much knowledge is required to work within a framework.

Yeah, there's a learning curve. But once you've are over the curve, things are pretty easy. I can usually get a "Minimum Viable Product" up and running in about a week.

That's kind of the idea with Randora-Framework. Once you understand where things go, it should, in theory, be trivial to make a "Minimum Viable Game".

I'm going to be off-line for the rest of the night. So, if you have any questions, don't wait around for me to answer them. I'll get around to it tomorrow though.