Skip to content
This repository has been archived by the owner on Nov 12, 2024. It is now read-only.

[Do not merge] Book form exercise #161

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[Do not merge] Book form exercise #161

wants to merge 2 commits into from

Conversation

tewain
Copy link
Contributor

@tewain tewain commented Mar 16, 2020

This is an exercise PR, it won't be merged to master. Contains two versions of solution, committed separately.

https://trello.com/c/7W1s8quQ

@@ -0,0 +1,6 @@
class Relation < ApplicationRecord
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small tip for future:

This name Relation is too generic.

The default convention for join table name is based on 2 table names:

books table and characters table have join table with name containing both table names in alphabetic order so books go first, then join table name is books_characters. Similar example here https://guides.rubyonrails.org/association_basics.html#the-has-and-belongs-to-many-association

The model for such join table would be BookCharacter but of course, we could use some custom name that's more explicit in domain problem as in the example with Appointments here https://guides.rubyonrails.org/association_basics.html#the-has-many-through-association

@@ -0,0 +1,29 @@
class BookCharactersCreator
def initialize
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can remove the definition of the empty constructor, it's by default like that without writing it :)

Copy link
Contributor

@ArturT ArturT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok.

@ArturT ArturT changed the title Book form exercise [Do not merge] Book form exercise Mar 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants