新手问题 ruby 代码写的 一陀了。。是不是应该反思啊~

jarorwar · 2013年04月18日 · 最后由 jarorwar 回复于 2013年04月22日 · 4721 次阅读

看到各种宣传 ruby 代码的优美可读等等~ 也算是刚开始学 ruby 吧(之前一直做 java,后来学了 python,觉得 python 才是真正的可读啊),最近用 ruby(纯的 ruby)做了一个简单的数据迁移工具,但是代码写的实在不忍心看啊~ 另外 ActiveRecord 这东西如何兼容现在已经有的数据库啊?

很菜很菜。望 big cow 指点一二啊~不胜感激~

我只回答第二个问题,代码质量的事真不是能教出来的,多看看好看的代码也许有帮助。

另外 ActiveRecord 这东西如何兼容现在已经有的数据库啊?

database.yml 里面可以配置 adapter

不妨把代码贴出来,看如何改进。

匿名 #3 · 2013年04月18日

代码都是一坨坨的,先把大坨改成小坨吧~ 至于优美不优美,是个见仁见智的问题。。。

#!/usr/bin/env ruby
# encoding: utf-8
#copy tag from test env to prod env
require 'mysql2'
class TagCopy
  def initialize
    @from_db = Mysql2::Client.new(:host => "192.168.xx.xx", :port => 3306, :username => "mysql", :password => "mysql", :database => "mydb")
    @to_db = Mysql2::Client.new(:host => "192.168.xx1.xx2", :port => 3306, :username => "root", :password => "root", :database => "mydb")
  end

  def get_tag_by_key(tag_key)
    #escape param to defend sql injection
    param = @from_db.escape(tag_key)
    sql = "" "SELECT id,no,tag_key,tag_name,tag_url,tag_desc,structure,type,isleaf,child,level, brand_no,category_no,channel_no,other_no,
                    row_num, site,sort_no, ad_quantity, image_size,parent_id,enable_more FROM tbl_cms_tags where tag_key = '#{param}' " "";
    puts sql
    results = @from_db.query(sql)
    uuid =@to_db.query("SELECT REPLACE(UUID(),'-','')", :as => :array)
    uuid = uuid.to_a[0]
    puts "total #{results.count} records "
    sql_root_id = "select id from tbl_cms_tags where tag_key ='ol_main'"
    rid = @to_db.query(sql_root_id)
    srid = rid.to_a[0]

    results.each do |row|
      #puts row.inspect
      add_tag(row,uuid[0],srid[0])
      if row["child"] >0
        getChild(uuid[0],row["id"])
      end
    end
    @from_db.close
    @to_db.close
  end

  def getChild(new_pid,parent_id)
    sql = "" "SELECT id,no,tag_key,tag_name,tag_url,tag_desc,structure,type,isleaf,child,level, brand_no,category_no,channel_no,other_no,
                    row_num, site,sort_no, ad_quantity, image_size,parent_id,enable_more FROM tbl_cms_tags where parent_id = '#{parent_id}' " "";
    results = @from_db.query(sql);

    results.each do |row|
      uuid =@to_db.query("SELECT REPLACE(UUID(),'-','')", :as => :array);
      uuid = uuid.to_a[0]
      add_tag(row,uuid[0],new_pid)
      #puts row.inspect
      if row["child"] >0
        getChild(uuid[0],row["id"])
      end

    end
  end


  def add_tag(row,id,parent_id)
    site = row["site"]?row["site"] : 0
    row_num = row["row_num"]?row["site"] : 1
    sort_no = row["sort_no"]?row["sort_no"] :0
    insert_sql ="" "INSERT INTO yitian_b2c_db.tbl_cms_tags
        (id,no,tag_key,tag_name,tag_url,tag_desc,structure,type,isleaf,child,level,brand_no,category_no,channel_no,other_no,row_num,site,sort_no,ad_quantity,image_size,parent_id,enable_more)
        VALUES
        ('#{id}','#{row["no"]}','#{row["tag_key"]}','#{row["tag_name"]}','#{row["tag_url"] }','#{row["tag_desc"] }','#{row["structure"] }',
        '#{row["type"] }','#{row["isleaf"] }',#{row["child"] },#{row["level"] },'#{row["brand_no"] }','#{row["category_no"] }','#{row["channel_no"] }',
        '#{row["other_no"] }',#{row_num},#{site},#{ sort_no  },0,#{row["image_size"]},'#{"#{parent_id}"}',#{row["enable_more"] }) " ""
    puts insert_sql
    @to_db.query(insert_sql)
  end
end

if __FILE__ == $0
  tc = TagCopy.new
  tc.get_tag_by_key("ol_index")
end

#2 楼 @chenge 代码已经贴出来了。欢迎大家点评,批评。鞭斥~ 只求长进~谢谢~

看帮助,学排版。

oh my god...

你应该代码格式化一下啊...

灰常感谢各位。已经格式化了 烦请@wppurking 删掉您的~,尽量让大家的讨论集中点。谢谢了~

大家可以把我这个代码作为最烂代码实践。来给新手普及下如何不要写出这么烂的代码~

这不是没用 ActiveRecord 么?

require 'active_record'

class TagCopy < ActiveRecord::Base
  # 如果已有数据库名字不对应
  set_table_name :old_tag_copies
  # 如果已有外键不对应
  has_many :tags, foreign_key: :old_tag_id
  # 如果已有类名不对应
  belongs_to :article, class_name: 'OldArticle'

  def method_1
  end

end

#11 楼 @kungs 因为是新手。加之 ActiveRecord 还没弄明白怎么样跟已有的数据库结合起来使用。所以就没用了。直接的写 sql 了。 哥们,谢谢你的例子。我抽空改用这种方式吧。

看到直接把 sql 写进代码里面我就晕了,ruby 有很多很多的 ORM 库的,为啥要直接插 sql 代码

#13 楼 @neverlandxy_naix 新手。我可以告诉你这是我的第二个在生产环境上使用的 ruby 程序么~在搞 rails ,但是还没太明白。所以,为了追求 “先把活干了” 所以就直接这么好了~ 好的东东,老兄推荐啊~不胜感激~

我提几点哈:

  1. mysql2 支持返回 row 能够通过 map 返回, 将查询 sql 语句调整为 select * 会一下好看很多.
  2. 将需要 insert 的字段使用 %w(id,no,tag_key), 然后对应的处理 row 用 each 也会好看一些
columns = %w(id no tag_ky)
insert_sql = "INSERT ..."
# set column
columns.each { |c| insert_sql << c }
# value
columns.each { |value| insert_sql << "value format" }

如果这是一个临时脚本, 排除很多的重复, 我基本能接受了.

补充下: 最后面的 if __FILE__ 判断在 Python 中需要 Ruby 中不需要.

#15 楼 @wppurking 谢谢,虽然只是一个脚本,但是在做数据迁移的时候就会用到,不是很频繁。但是会不定期持续使用,再者本人觉得代码实在是太烂了。所以就发上来,烦请各位大大帮忙找 bug,我会进一步按照各位的 建议去持续修改代码~。虽然是小东西,但是里面所包含的一些问题是通用的,因为刚开始学,对很多东西不是很确定。而各位的建议正好给我一个方向,指出了我的错误,帮助我成长~非常感谢~

这是一个 code review 的需求,推荐使用 http://chopapp.com/

楼主的 php 代码写的不错呀

#17 楼 @zlx_star 不错,mark 一下

同样是 Java 转 Ruby 我的建议是: 首先 不推荐上来就写这么复杂功能的东西 应该先把书看完 第一本:Ruby Programming 然后 能写一行的 不要写两行

uuid =@to_db.query("SELECT REPLACE(UUID(),'-','')", :as => :array)
uuid = uuid.to_a[0]

直接写成 uuid =@to_db.query("SELECT REPLACE(UUID(),'-','')", :as => :array).to_a[0] 不好么... 第三 看下 Ruby 风格指南 http://stylesror.github.io

最后 祝你早日拜托屎一样的一坨坨...

#17 楼 @zlx_star 非常感谢~我去看一下~

#19 楼 @zlx_star 兄弟,感谢感谢了啊~这个代码先放一下。我要去搞 ruby 跟 dubbo 的 hessian 通讯了~回头我会按照各位的建议发了一个优化版本,然后大家继续鞭斥~

#21 楼 @zj0713001 两行看起来更习惯(个人)

书是吞了一本 rails 的。看得没那么仔细。没有很多时间去学习,只能边学边搞了~

总的来说灰仓感谢~

你这不是用 java 的写法写 ruby 吗?注册会话,打开链接,创建会话,执行 sql,遍历结果,呵呵,如果用上 orm 是否好点…

#24 楼 @jarorwar 约定大于配置啊 两行你看起来更习惯 但是大部分 Rubyist 都不习惯啊 所以我觉得这是 优美可读与一坨 的一个矛盾所在... :)

太多了没看完,我就说一点

@from_db = Mysql2::Client.new(:host => "192.168.xx.xx", :port => 3306, :username => "mysql", :password => "mysql", :database => "mydb")

我会写成

@source_db = Mysql2::Client.new(
  host: "192.168.xx.xx",
  port: 3306,
  username: "mysql", 
  password: "mysql", 
  database: "mydb"
)

看起来清楚点

#24 楼 @jarorwar

#!/usr/bin/env ruby
# encoding: utf-8
#copy tag from test env to prod env
require 'mysql2'
class TagCopy

    CMS_COLUMNS = %w[id no tag_key tag_name tag_url tag_desc structure type isleaf child level brand_no category_no channel_no other_no row_num site sort_no ad_quantity image_size parent_id enable_more].join(',')

    def initialize
        @from_db = Mysql2::Client.new(:host => "192.168.xx.xx", :port => 3306, :username => "mysql", :password => "mysql", :database => "mydb")
        @to_db = Mysql2::Client.new(:host => "192.168.xx1.xx2", :port => 3306, :username => "root", :password => "root", :database => "mydb")
    end

    def get_tag_by_key(tag_key)
        results = @from_db.query("SELECT #{CMS_COLUMNS} FROM tbl_cms_tags WHERE tag_key = '#{@from_db.escape(tag_key)}'")
        uuid = @to_db.query("SELECT REPLACE(UUID(),'-','')", :as => :array).to_a[0]
        puts "total #{results.count} records "
        srid = @to_db.query("SELECT id FROM tbl_cms_tags WHERE tag_key = 'ol_main'").to_a[0]

        results.each do |row|
            #puts row.inspect
            add_tag(row, uuid[0], srid[0])
            getChild(uuid[0], row["id"]) if row["child"] > 0
        end
        @from_db.close
        @to_db.close
    end

    def getChild(new_pid, parent_id)
        results = @from_db.query("SELECT #{CMS_COLUMNS} FROM tbl_cms_tags WHERE parent_id = '#{@from_db.escape(parent_id)}'");

        results.each do |row|
            uuid = @to_db.query("SELECT REPLACE(UUID(),'-','')", :as => :array).uuid.to_a[0]
            add_tag(row, uuid[0], new_pid)
            getChild(uuid[0], row["id"]) if row["child"] >0
        end
    end


    def add_tag(row, id, parent_id)
        row.merge!('id' => id, 'site' => row["site"] || 0, 'row_num' => row["row_num"] ? row["site"] : 1, 'sort_no' => row["sort_no"] || 0)
        insert_sql = "INSERT INTO yitian_b2c_db.tbl_cms_tags (#{row.keys.join(',')},ad_quantity) VALUES (#{row.values.map{|v| "'#{v}'"}.join(',')},0)"
        puts insert_sql
        @to_db.query(insert_sql)
    end
end

if __FILE__ == $0
    tc = TagCopy.new
    tc.get_tag_by_key("ol_index")
end

简单帮你改了改 肯定还能优化的 我没运行起来 可能会有小错误 只能你自己小调试啦 没这么多时间哈~

#26 楼 @zj0713001 哦。好的。以后改正,谢谢~

#28 楼 @zj0713001 非常感谢,您的代码真是不错。我就膜拜了~

最大的问题是裸写 SQL,难阅读容易出错。用 ActiveRecord 就会改善很多,我看不懂逻辑就不给范例了。

#!/usr/bin/env ruby
# encoding: utf-8
#copy tag from test env to prod env
require 'mysql2'
class TagCopy

  def initialize
    @from_db = Mysql2::Client.new(
      :host     => "192.168.xx.xx", 
      :port     => 3306, 
      :username => "mysql", 
      :password => "mysql", 
      :database => "mydb"
    )
    @to_db = Mysql2::Client.new(
      :host     => "192.168.xx1.xx2", 
      :port     => 3306, 
      :username => "root",
      :password => "root", 
      :database => "mydb"
    )
  end

  # 将 get_tags_by_key 改名为 migrate_tags_by_key
  # 原因是: get_xxxx 约定中都是读取操作,不会
  # 修改数据,无论 get 多少次,都不会引起副作用
  # 不适合这里的功能
  def migrate_tags_by_key( tag_key )
    #escape param to defend sql injection
    param = @from_db.escape(tag_key)

    # 我觉得 select * 在这里就足够了,等性能有问题了再考虑优化
    sql = "SELECT * FROM tbl_cms_tags where tag_key = '#{param}'";
    puts sql

    results = @from_db.query(sql)
    puts "total #{results.count} records "

    results.each do |row|
      # 这里将 uuid 查询,root_id 重构成了一个方法
      # 方便阅读。同时带来额外的好处:在其他方法中
      # 可以重用这个逻辑了

      # 为什么叫 new_parent_id 而不是 new_pid ?
      # 因为 pid 大家已经习惯了是 process id
      add_tag( row, new_parent_id, first_sql_root_id)

      if has_children?(row)
        migrate_children( new_parent_id, row['id'] )
      end
    end
  end

  # 注意到了没, add_tag 现在在 migrate_children 之前
  # 定义? 是因为 migrate_tags_by_key 方法中先调用了
  # add_tag 方法。函数的定义顺序遵循他们出现的顺序,
  # 让后面的维护者更通顺地阅读
  def add_tag(row, id, parent_id)
    row = row.dup
    row["site"]     ||= 0
    row["row_num"]  ||= 1
    row["sort_no"]  ||= 0

    # 把你需要的属性列在这里,不要去重复写
    # 两遍属性。原先的SQL很容易写错,这样写
    # 就不容易犯错,出错了也很容易辨认
    attrs = %w[ 
        id          no          tag_key 
        tag_name    tag_url     tag_desc 
        structure   type        isleaf 
        child       level       brand_no 
        category_no channel_no  other_no 
        row_num     site        sort_no 
        ad_quantity image_size  parent_id 
        enable_more
    ]

    values = attrs.map {|attr| row[attr] }

    # 这样写,这条sql语句就没有那么可怕了
    # 如果一行代码超过了70个字符,很危险的标志
    insert_sql = <<-SQL 
      INSERT INTO yitian_b2c_db.tbl_cms_tags
        (#{attrs.join ','})
        VALUES
        (#{values.join ','})
      SQL

    puts insert_sql
    @to_db.query(insert_sql)
  end

  def migrate_children( new_parent_id, parent_id)
    sql = "SELECT * FROM tbl_cms_tags where parent_id = '#{parent_id}'";

    @from_db.query(sql).each do |row|
      add_tag(row, new_parent_id, new_parent_id)
      if has_children?(row)
        migrate_children( new_parent_id, row["id"])
      end
    end
  end

  def cleanup
    @from_db.close
    @to_db.close
  end

  private

    def uuid
      q = "SELECT REPLACE(UUID(),'-','')"
      @uuid ||= @to_db.query( q, :as => :array).first
    end

    def new_parent_id
      uuid.first
    end

    def first_sql_root_id
      sql_root_id.first
    end

    def sql_root_id
      q = "select id from tbl_cms_tags where tag_key ='ol_main'"
      @sql_root_id ||= @to_db.query( q ).first
    end

    def has_children?(row)
      row['child'] > 0
    end

end

if __FILE__ == $0
  tc = TagCopy.new
  tc.migrate_tags_by_key "ol_index"

  # 将 close 单独独立成一个方法
  # 本意是使核心方法更加简洁, 发现带来了额外的好处:
  # 这个类现在可以支持一次migrate多个任务
  tc.cleanup
end

对于核心数据结构不是很清楚,只做了些语言层面的重构

只做了原理性质的重构,里面具体的操作上估计有问题,比如 sql 的拼接 LZ 的新旧数据库我发现 column name 都是一样的,这样完全不必要硬写 sql 的;如果不一样的话弄个映射也可以避免; 此外重构就是把处于不同抽象层次上的逻辑封装到不同的方法中。

发现和楼上的基本重复了。。。

#!/usr/bin/env ruby
# encoding: utf-8
#copy tag from test env to prod env

require 'mysql2'

class TagCopy

  def transfer_tag_by_key(tag_key)
    open_db

    results = load_old_tags(tag_key)

    uuid = get_uuid_from_new_db
    srid = get_srid_from_new_db

    results.each do |old_tag|
      transfer_tag old_tag, uuid[0], srid[0]
      transfer_child uuid[0],old_tag["id"] if has_child(old_tag)
    end

    close_db
  end

  def transfer_child new_pid, parent_id
    results = load_old_childs_for_parent(parent_id)

    results.each do |row|
      uuid = get_uuid_from_new_db
      transfer_tag(row,uuid[0],new_pid)
      transfer_child uuid[0],row["id"] if has_child(row)
    end
  end


  def transfer_tag(row, id, parent_id)
    miss_colums = %w(site row_num sort_no ad_quantity)
    miss_colums_values = row["row_site"] || 0, row["row_num"] || 1, row["sort_no"] || 0, 0

    all_columns = ["id", "parent_id", common_columns, miss_colums].flatten.join
    all_columns_value = [id, parent_id, common_columns.collect{|col| row[col]}, miss_colums_values].flatten

    insert_sql = "insert into yitian_b2c_db.tbl_cms_tags (#{all_columns.join(',')}) values (#{all_columns_value.join(',')})"

    @to_db.query(insert_sql)
  end

  private

  def has_child
    row["child"] >0
  end

  def common_columns
    %w(no tag_key tag_name tag_url tag_desc structure type isleaf child level brand_no category_no channel_no other_no enable_more image_size)
  end

  def old_db
    Mysql2::Client.new {:host => "192.168.xx.xx",  
                        :port => 3306, 
                        :username => "mysql", 
                        :password => "mysql", 
                        :database => "mydb"
                       }
  end

  def new_db
    Mysql2::Client.new {:host => "192.168.xx1.xx2", 
                        :port => 3306, 
                        :username => "root", 
                        :password => "root", 
                        :database => "mydb"
                       }
  end

  def load_old_tags(tag_key)
    param = @from_db.escape(tag_key)
    sql = "select #{common_columns.join(',')} from tbl_cms_tags where tag_key = '#{param}'"
    @from_db.query(sql)
  end

  def load_old_childs_for_parent(parent_id)
    sql = "select #{common_columns.join(',')} from tbl_cms_tags where parent_id = '#{parent_id}' "
    results = @from_db.query(sql);
  end

  def get_uuid_from_new_db
    @to_db.query("SELECT REPLACE(UUID(),'-','')", :as => :array).to_a[0]
  end

  def get_srid_from_new_db
    @to_db.query("select id from tbl_cms_tags where tag_key ='ol_main'").to_a[0]
  end

  def close_db
    @from_db.close
    @to_db.close
  end

  def open_db
    @from_db ||= from_db
    @to_db ||= to_db 
  end
end


TagCopy.new.transfer_tag_by_key("ol_index")

#33 楼 @donnior 握爪,学习~

#30 楼 @jarorwar 又有两个大牛给你范例了哈 他们改的更彻底了 学学他们的哈

#32 楼 @qhwa #33 楼 @donnior 二位大牛重构的真不错哈

#30 楼 @jarorwar 我是按照你的 Java 思路来改的 他们二位是按照重构的思路来改的 我觉得重构思路你理解起来会费劲 所以先打好语言基础哈~ 同是 Java 转的 我当时也非常的痛苦

#34 楼 @qhwa 共勉,😄 你比俺厉害,都尝试去了解他里面的业务逻辑了,比如那个 uuid;我也看了下,原先的逻辑有些不太直接,不过考虑到跟他的具体场景相关,就没有做推断和改变了。

提醒一下各位,可以去标喜欢

我写的也类似,哈哈, 不过我经常是用 csv 文件。而且我一般是脚本,大多是从 html 网页中提取数据,然后处理,然后存储在 csv 文件中。产品经理写的脚本更可怕....是流线型的。从头到尾跑下来,完成任务就拉倒了。

#36 楼 @zj0713001 非常感谢。我又在忙着造另外一个脚本呢。所以这个脚本暂时没时间改造了。但是后期必须改造的 另外@donnior 兄这个改造是相当牛了。从 method 的 name 都帮我改了。灰仓之感谢~

#39 楼 @mobiwolf 脚本的任务就是 automation 的啊。自动跑完的。不然也没多大意思了~呵呵。另外弱弱的问下。流线型是什么 style?

#41 楼 @jarorwar 呵呵,产品经理的流线型就是所谓结构化编程,从第一行开始运行,到最后一行。基本没有类定义,最多整几个全局变量....然后代码中各种很类似东西,有时候会把类似的抽象成一个函数,然后需要的地方调用。有时候就那样复制过来了....哈哈。惨不忍睹....

用简单粗暴的方法跑起来先把任务完成就是好的开始

PHP 代码不错啊!

#44 楼 @diga2005 额。这更像 java 代码不是么?

需要 登录 后方可回复, 如果你还没有账号请点击这里 注册