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
【課題】AmazonのURL変換 & CSV出力 #3
base: main
Are you sure you want to change the base?
Conversation
lib/convert_url.rb
Outdated
] | ||
|
||
decoded_url = url_array.map do |url| | ||
item_data = url[25..-1].gsub!('/', ',') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
25文字というのはちょいと脆いですね。ドメイン変わった瞬間動かなくなるので。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
確かに変化に対応できない実装ですね。
修正しました 694c0d1
.byebug_history
Outdated
exit | ||
du | ||
n | ||
next | ||
item_data | ||
exit | ||
item | ||
n | ||
exit | ||
item_data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
また追加されちゃってます。
プルリクをあげる際に毎回Files changed
を確認する癖をつけた方が良いですね。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
失礼しました。上げる前に毎回確認するようにいたします
0e97172
lib/convert_url.rb
Outdated
amazon_csv = CSV.generate do |csv| | ||
decoded_url.each do |du| | ||
csv << %w[商品ID 商品名] | ||
csv << [du[3], du[5]] | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
細かいですが、これってCSVファイル作られてますっけ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generateではなくopen に修正しました。
e86acb6
lib/convert_url.rb
Outdated
decoded_url = url_array.map do |url| | ||
item_data = url.split('/') | ||
item_data[3] = CGI.unescape((item_data[3]).to_s) | ||
item_data | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
個人的には多次元配列は極力使いたくないですね。わかりづらくなるので。
やるなら
[
{ id: "xxx", url: "xxx" },
{ id: "xxx", url: "xxx" },
{ id: "xxx", url: "xxx" },
]
のようなidとurlをキーに持つハッシュの配列にした方が自然かなと思います。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
多次元配列は強力使わないようにします。
今回は、ハッシュの配列で対応いたしました。
e86acb6
item_data = { | ||
id: url_data[5], | ||
url: CGI.unescape((url_data[3]).to_s) | ||
} | ||
item_data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここも細かいですが、わざわざitem_data
という変数に格納する必要はないですね。
{
id: url_data[5],
url: CGI.unescape((url_data[3]).to_s)
}
だけで良いと思いますよ。
「AmazonのURL変換 & CSV出力」の課題が終了しました。
レビューをお願いいたします。