06. Често стрещани грешки в домашните

06. Често стрещани грешки в домашните

06. Често стрещани грешки в домашните

31 октомври 2011

Днес

Трета задача

Трета задача

малко особености

Втора задача

Грешка #0

Типичните

Грешка #1

Не ползва Enumerable

@collection = []

songs.each do |name, artist, genres_string, tags_string|
  genre, subgenre = genres_string.split(/,\s*/)
  tags = artist_tags.fetch(artist, [])
  tags += [genre, subgenre].reject(&:nil?).map(&:downcase)
  tags += tags_string.split(/,\s*/) unless tags_string.nil?

  @collection << Song.new(name, artist, genre, subgenre, tags)
end

Ползва #each вместо #map

Източник

Грешка #1

Всеки път, когато тръгнете да пишете:

somethings = []

things.each do |thing|
  ...
  somethings << modified_thing
end

Всъщност имате предвид:

somethings = things.map do |thing|
  ...
end
"Всеки не-функционален #each е пирон в ковчега на капитализма."

Грешка #2

лошо подравнен код

def matchCriteria(criteria)
  name? criteria[:name] and
  artist? criteria[:artist] and
  tags? criteria[:tags] and
  filter? criteria[:filter] and
  true
end
Източник

Грешка #2

По-правилно подравняване:

def matchCriteria(criteria)
  name? criteria[:name] and
    artist? criteria[:artist] and
    tags? criteria[:tags] and
    filter? criteria[:filter]
end

name?, artist? и прочее не са добри имена.

Грешка #3

Не гледате подсказките

all_tags = tags.kind_of?(String) ? [tags] : tags

Всъщност искате да напишете:

all_tags = Array(tags)
Източник

грешка #4

много повторение

def parse_single_song(song_as_string, artist_tags)
  attributes = song_as_string.split('.')
  name = attributes[0].strip
  artist = attributes[1].strip
  genre = attributes[2].split(',')[0].strip
  subgenre = attributes[2].split(',').fetch(1,'').strip
  tags = attributes.fetch(3, '').split(',').map{|attr| attr.strip}
  additional_tags = artist_tags.fetch(artist, [])
  Song.new(name, artist, genre, subgenre, tags|additional_tags)
end

Тук всякаш има твърде много, твърде сбит код. Някои елементи в него сеповтарят визуално (attributes[x].strip например)

Източник

Грешка #4

Много повторение

Ето как може да стане по-кратко:

def parse_single_song(song_as_string, artist_tags)
  name, artist, genres, tags = song_string.split('.').map(&:strip)
  genre, subgenre            = genres.split(',').map(&:strip)
  tags                       = tags.split(',').map(&:strip)

  tags += additional_tags[artist] if additional_tags.has_key? artist

  Song.new name, artist, genre, subgenre
end

DRY

Don't Repeat Yourself

Просто правило: Не повтаряйте информация в кода си.

Всеки път, когато откриете повторение в кода си, намирайте начин да го елиминирате. Това е най-лесния начин да се научите да пишете добър код.

Грешка #5

Странно разбиване на методи

def matches?(criteria)
  if (criteria[:name] != nil) and (check_name(criteria[:name]) == false)
    return false
  end
  if (criteria[:artist] != nil) and (check_artist(criteria[:artist]) == false)
    return false
  end
  # ...
  true
end

def check_name(name)
  @name.eql?(name)
end

def check_artist(artist)
  @artist.eql?(artist)
end

Границата между matches? и check_xxx е странна.

По-добре или проверката за nil да се изнесе в check_xxx методите, или да се inline-нат в matches?

Източник

Грешка #5

Странно разбиване на методи

Отделно:

def matches?(criteria)
  if (criteria[:name] != nil) and (check_name(criteria[:name]) == false)
    return false
  end
  # ...
  true
end

def check_name(name)
  @name.eql?(name)
end

#eql?

Как обектите ни да бъдат ключове в хеш? Може да пробваме така, но няма да сработи:

class Cell
  attr_reader :x, :y

  def initialize(x, y)
    @x, @y = x, y
  end

  def hash
    [@x, @y].hash
  end

  def ==(other)
    other.is_a?(Cell) and x == other.x and y == other.y
  end
end

board = {Cell.new(0, 0) => true, Cell.new(0, 1) => true, Cell.new(0, 2) => true}
Cell.new(0, 0) == Cell.new(0, 0) # true
board[Cell.new(0, 0)]            # nil

#eql? (2)

Правилния начин:

class Cell
  attr_reader :x, :y

  def initialize(x, y)
    @x, @y = x, y
  end

  def hash
    [@x, @y].hash
  end

  def eql?(other)
    other.is_a?(Cell) and x == other.x and y == other.y
  end
end

board = {Cell.new(0, 0) => true, Cell.new(0, 1) => true, Cell.new(0, 2) => true}
board[Cell.new(0, 0)] # true

#eql? (3)

Грешка #6

Array() отново

def check_tags(tags)
  if (tags.kind_of?(String))
    check_tag(tags)
  else
    tags.all? {|tag| check_tag(tag)}
  end
end

Всъщност:

def check_tags(tags)
  Array(tags).all? { |tag| check_tag(tag) }
end
Източник

Грешка #7

Enumerable отново

def matches_criteria?(criteria_hash)
  criteria_hash.each do |attribute, value|
    return false unless matches_criterion? attribute, value
  end
  true
end

Всъщност:

def matches_criteria?(criteria)
  criteria.all? { |attribute, value| matches_criterion? attribute, value }
end
Източник

Грешка #7

Enumerable отново (2)

Всяко:

things.each do |thing|
  return false unless condition?(thing)
end
true

Всъщност е:

things.all? { |thing| condition?(thing) }

Аналогично за #any? ако се разменят true и false и unless стане if.

Не ползвате Enumerable

Грешка #8

Гаден whitespace

@tags = song_hash[:tags].map!{|tag| tag.strip}
tags.select { |tag| not tag.end_with?'!' }.all?{ |tag| @tags.include?tag }
pairs = [ :name, :artist, :genre_subgenre, :tags ].zip(values)

Whitespace-а се поставя така:

@tags = song_hash[:tags].map! { |tag| tag.strip }
tags.select { |tag| not tag.end_with? '!' }.all? { |tag| @tags.include? tag }
pairs = [:name, :artist, :genre_subgenre, :tags].zip(values)
Източник

Грешка #9

Мутиращи операции

@tags = song_hash[:tags].map! { |tag| tag.strip }

#map! е глупаво, Евгени. Какво си пушил, докато си го писал?

По добре ще стане така:

@tags = song_hash[:tags].map { |tag| tag.strip }
@tags = song_hash[:tags].map(&:strip)
Източник

Грешка #9

Мутиращи операции

@tags = song_hash[:tags].map! { |tag| tag.strip }

Какви проблеми виждам:

Mutable vs. Immutable

Грешка #10

Странни имена на методи

def self.fix_hash(song_hash, artist_tags)
    song_hash[:genre_subgenre] = song_hash[:genre_subgenre].split /\s*,\s*/
    song_hash[:genre], song_hash[:subgenre] = song_hash[:genre_subgenre]
    song_hash[:tags] = song_hash[:tags] ? song_hash[:tags].split(',') : []
    song_hash[:tags] << song_hash[:genre].downcase if song_hash[:genre]
    song_hash[:tags] << song_hash[:subgenre].downcase if song_hash[:subgenre]
    song_hash[:tags] += (artist_tags[song_hash[:artist]] or [])
    song_hash
end

fix? Какво му е счупеното?

Източник

Грешка #11

Страннен синтаксис

def check_tag?(tag)
  if tag.end_with? '!' then
    return false if @tags.include? tag.chop
  else
    return false unless @tags.include? tag
  end

  return true
end

then-а в този if е излишен

Източник

Грешка #12

Лоша проверка за тип

def filter_by_tags(songs, tags)
  if tags.class == String then
    songs.select { |song| song.check_tag? tags }
  else
    songs.select { |song| song.check_tags? tags }
  end
end

Грешка #13

Методи, които правят две неща

class Song
  # ...
  def name(compare_with = nil)
    compare_with.nil? ? @name : @name == compare_with
  end

  def artist(compare_with = nil)
    compare_with.nil? ? @artist : @artist == compare_with
  end
end

По-добре щеше да стане, ако бяха matches_name? и matches_name

Източник

Грешка #14

Подписи из кода

class Song
  def filter(saint_lambda)
    saint_lambda.call self
  end
end

Грешка #14

"Сладък код"

Assembler код из Code Complete, (стр. 792):

AX, 723h  ; R. I. P. L. V. B.

Грешка #15

Monkey patch на вградени класове

class Array
    def dc
        self.map(&:downcase)
    end
    def split arg
        self
    end
    alias has? include?
    alias d_if delete_if
end
class String
    alias dc downcase
    def trim
        self.gsub(/( ){2,}/,'')
    end
end

Не. Просто не. Плюс, лоша идентация.

Източник

Грешка #16

if then else end на един ред

subgenre = if genre_sub.length > 1 then genre_sub[1] else nil end

Пише се или така:

subgenre = if genre_sub.length > 1
  genre_sub[1]
else
  nil
end

Или така:

subgenre = genre_sub.length > 1 ? genre_sub[1] : nil
Източник

Грешка #17

if then end на един ред

def initialize attributes
  @name = if attributes[0] != nil then attributes[0].strip end
  @artist = if attributes[1] != nil then attributes[1].strip end
  @genre = if attributes[2] != nil then (attributes[2].split ",") [0].strip end
  @subgenre = if attributes[2] != nil then (attributes[2].split ",") [1] end
  if @subgenre != nil then @subgenre.strip! end
  if @genre != nil then @tags = Array (@genre.downcase) end
  if @subgenre != nil then @tags << @subgenre.downcase.strip end
  if attributes[3] != nil 
    (attributes[3].split ",").each { |tag|  @tags << tag.strip }
  end
end

Доста по-екстремно.

Източник

Грешка #17

if then end на един ред

Грешка #18

require на странно място

class Collection 
  require 'set'

  attr_accessor :songs

  def initialize (songs_as_string, artist_tags)
    # ...
  end

  # ...
end

Не че сме ви говорили за require, но ако го ползвате, той трябва да стои най-горе.

Впрочем, между initialize и скобата не трябва да има празно място.

Грешка #19

неспазване на конвенции

class Collection
  def initialize(string, stringTags)
   @arrSongs = []
   arrArtist = []
   i=0
   string.each_line { |n| @arrSongs.push Song.new(n) }
   @arrSongs.each do |n| 
   arrArtist.push n.artist
   end
   arrArtist.each do |value|
   if stringTags.has_key? (value)
      @arrSongs[i].change_tags (stringTags[value].to_s.gsub('"','')) 
   end
   i+=1
   end
  end
Източник

Грешка #19

неспазване на конвенции

Какви грешки имаше тук?

Грешка #19

неспазване на конвенции

def find(criteria)
  result = @music
  criteria.each { |key, value| 

    if key == :tags
      result = @music.select { |song|
        # not working 
        (song.send(key) & value.split).sort == value.split.sort }

    elsif key == :filter
      result = @music.select { |song| value.call song }

    else
      result = @music.select { |song| song.send(key) == value }
    end
  }
  result
end
Източник

Грешка #20

Странно подравняване

def initialize(song_attributes,artist_tags)
  @name = song_attributes[0].strip
      @artist = song_attributes[1].strip
  n = song_attributes[2].split(", ")
  @genre = n[0].strip
  if n[1].nil? then @subgenre = nil
  else @subgenre = n[1].chomp
  end
  @tags = []
  if song_attributes[3] != nil then @tags << song_attributes[3].split(", ")
  end
  @tags << @genre.downcase << @subgenre      
  @tags.delete_if {|x| x == nil}.flatten!
  @tags.each {|n| n.strip!}.map{|n| n.downcase!}
  artist_tags.each do|key,value| if key==@artist then @tags<<value.flatten!
  end
  end
end
Източник

Feature Envy

code smell

class Collection
  def filter_by_name(songs, name)
    songs.select { |song| song.name == name }
  end

  def filter_by_artist(songs, artist)
    songs.select { |song| song.artist == artist }
  end

  def filter_by_lambda(songs, filter)
    songs.select(&filter)
  end

  def filter_by_tags(songs, tags)
    if tags.class == String then
      songs.select { |song| song.check_tag? tags }
    else
      songs.select { |song| song.check_tags? tags }
    end
  end
end
Източник

Обяснение на предишния слайд

Принцип: Single Responsibility Principle

Shotgun surgery

code smell

Принципи и миризми

Въпроси