Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

メモアプリの作成(DB版) #3

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

メモアプリの作成(DB版) #3

wants to merge 12 commits into from

Conversation

mami-inuzuka
Copy link
Owner

プルリクエストを作り直しました

app.rb Outdated
enable :method_override

def connect_db
@conn = PG.connect(dbname: 'sinatra_note_app')
Copy link

Choose a reason for hiding this comment

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

接続をキャッシュしているの素晴しいですね。大体ここが指摘ポイントになるので。
ただ、呼び出し元で nil チェックをしているので、呼び出す度にifが必要になっているのがもったいないですね。
このメソッド内でチェックするようにして、 メソッド名をconn とか connectionとかにして、@connを返すようにすれば呼び出し元はシンプルになりますよ。

Copy link
Owner Author

@mami-inuzuka mami-inuzuka Aug 28, 2021

Choose a reason for hiding this comment

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

レビューありがとうございます!

なるほど!確かに、同じif文が繰り返されているのをみて「共通化できないかな」と考えてみるべきでした...!

修正してみたのでご確認お願いします😊🙏

enable :method_override

def connection
@conn = PG.connect(dbname: 'sinatra_note_app') if @conn.nil?
Copy link

Choose a reason for hiding this comment

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

惜しい!

あと一歩なので書いちゃうと、これを

def connection
  @conn = PG.connect(dbname: 'sinatra_note_app') if @conn.nil?
  @conn
end

とすれば

  connection
  @conn.exec("INSERT INTO notes (title,content) VALUES ('#{params[:title]}','#{params[:content]}')")

  connection.exec("INSERT INTO notes (title,content) VALUES ('#{params[:title]}','#{params[:content]}')")

となります。

あとはイディオムとして以下のようによく書きます。 ||= で nil(もしくはfalse)のときだけ右辺を実行して代入させる事ができるんですよね。

def connection
  @conn ||= PG.connect(dbname: 'sinatra_note_app') 
end

Copy link
Owner Author

Choose a reason for hiding this comment

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

レビューありがとうございます🙇🏻‍♂️!!

なるほど、確かにその方がより簡潔になりますね...!!
また||= についてもよく見かける書き方ですが、今回の例でより使いどころを理解することができました!使いこなせるようになりたいと思います!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants