vidubrovskyi / sports-communication-network

0 stars 0 forks source link

General #2

Open RoRElessar opened 2 years ago

RoRElessar commented 2 years ago

When you work in Rubymine or other IDEs from Jet Brains, always add to .gitignore .idea directory, because it contains unnecessary information for the project and your individual IDE settings for a current project.

You're using sqlite3 database here. It'll be better to use postgres. In real life there are not so many projects that use sqlite. Also, don't forget to add database.yml to .gitignore. A good practice will be creating a database.yml.example file in config directory with settings for a db connection. You can work with several people, or even with a big team on the same project, and all of them could have different settings for their local databases.

You have generated some devise controllers but didn't overwrite default methods there. There is no need to keep them.

You're using double quotes for strings. There is a Rails convention to use double quotes for string interpolation. For example: "Hello #{current_user.name}", but use single quotes for a simple text - 'Hello, World!'

The naming of variables is very important. Don't use contractions, if it is a contact, use contact = Contact.first, but never use c = Contact.first. When you meet a variable in the code - it should be clear what it means. You can use contraction for index, in this case, it's ok.

Spaces. In if / else block use 2 spaces to ident the code that is executed when condition is true or false.

Here is a link to a ruby style guide: https://github.com/rubocop/ruby-style-guide

Don't use several empty lines to separate your code.

<%= link_to (user_path(c.user_id)) do %> <%= User.find(c.user_id).name %> <%= User.find(c.user_id).last_name %> <% end %> If you're using a block - spread it into several lines. You can also move this code to a helper, to make your view look clear and lean, or create a method in a User model to display a full name. Or you can combine a helper with a method in the model.

<% if user_signed_in? and current_user.workgroup == "Admin" || current_user.id == c.user_id %> Don't use and, use && operator instead.

In _chat_block.html.erb you have this loop <% for c in @room.chats %> Why are you using for instead of each?

Don't leave empty helpers. If you don't use any - just delete them.

vidubrovskyi commented 2 years ago

First of all, thanks for reviewing.

  1. When you work in Rubymine or other IDEs from Jet Brains, always add to .gitignore .idea directory, because it contains unnecessary information for the project and your individual IDE settings for a current project. - Didn't know, I'll add

  2. Here you are using sqlite3 database. It's better to use postgres. In real life, there are not many projects that use sqlite. Also don't forget to add database.yml to .gitignore. It is good practice to create a database.yml.example file in the config directory with the settings for connecting to the database. You may be working with multiple people or even a large team on the same project, and they may all have different settings for their local databases. - I will study this

  3. You have generated some devise controllers but didn't overwrite default methods there. There is no need to keep them. - Ok

  4. You're using double quotes for strings. There is a Rails convention to use double quotes for string interpolation. For example: "Hello #{current_user.name}", but use single quotes for a simple text - 'Hello, World!' - I didn't know, I read that "" is universal, so I used it all the time

  5. The naming of variables is very important. Don't use abbreviations if it's a contact, use contact = Contact.first, but never use c = Contact.first. When you meet a variable in the code, it should be clear what it means. You can use a shorthand for the index, in which case it's ok. - Ok

  6. Spaces. In if / else block use 2 spaces to ident the code that is executed when condition is true or false. - Seems like I am doing so, maybe I have made a mistake some where

  7. <%= link_to (user_path(c.user_id)) do %> <%= User.find(c.user_id).name %> <%= User.find(c.user_id).last_name %> <% end %> If you're using a block - spread it into several lines. You can also move this code to a helper, to make your view look clear and lean, or create a method in a User model to display a full name. Or you can combine a helper with a method in the model. - Ok

  8. <% if user_signed_in? and current_user.workgroup == "Admin" || current_user.id == c.user_id %> Don't use and, use && operator instead. - Ok

  9. Don't leave empty helpers. If you don't use any - just delete them. - Ok