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

31 октомври 2011

Днес

Весела коледа! (31 oct = 25 dec).
Ще разгледаме решенията ви на втора задача.
Първия SOLID принцип.
Code smells.

Трета задача

Публикувана е.
По-трудна е от предишните две.
Започнете да я решавате скоро.
Няма да стават по-лесни.

Трета задача

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

Споделяме (почти) пълния тест, с който ще тестваме.
Ще напишем по-подробни подсказки как да я реализирате.
Точните ограничения не са решение още, но има предварителни.
2 нива на влагане.
8 реда на метод.
10 метода в клас.

Втора задача

Оценки по-късно през седмицата.
Бонус точки за добър стил.
Вземаме точки за лош.
Постепенно ще коментираме всички решения.
Предавайте задачи по-рано да получите feedback.

Грешка #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

matchCriteria не е по конвенция.
Изглежда като пет отделни израза, а всъщност е един.
Изглежда че връща true
Впрочем, and true?

Източник

Грешка #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

criteria[:name] вместо criteria[:name] != nil
not check_name(criteria[:name]) вместо check_name(criteria[:name]) == false
Какво прави Object#eql?

#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)

Трябва да дефинирате #eql?
Обикновено се прави така: alias eql? ==
Трябва да предефинирате #hash по стандартния начин.
Стандартния начин беше: a.eql?(b) → a.hash == b.hash
Стрелката значи импликация.

Грешка #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

Отново: Enumerable е критичен за Ruby.
Дава ви много удобства, с които да пишете по-малко.
По-важното: прави кода по-ясен.
В горния пример, #all? е по-ясен от #each.
Всеки пропуск да ползвате #map, #select, #any?, и #inject и прочее е лош стил.

Грешка #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 }

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

Мутира параматър на Collection.find.
Не е ясно дали клиента го очаква.
Не е ясно дали клиента зависи от това поведение.
Ако едно от двете е вярно, може !-та да не се забележи.
Ако нито едно не е вярно, !-та е объркваща.
Сериозно, как ти хрумна?

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

Първо, грешно е. Може да бъде Proc, а не lambda.
Второ, мястото на подписите не е в кода, а в git историята.
Трето, виж първо и второ.

Грешка #14

"Сладък код"

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

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

Кода прави фатална грешка на този ред.
Коментара е криптичен.
723 в шестнадесетична и e 1827 в десетична.
Лудвиг ван Бетовен е починал през 1827.
Ааахааа...

Грешка #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 на един ред

if condition? then 42 end връща nil ако condition? е неистина.
Въпреки това, криво е когато кодът зависи от тази особенност.
Може да напишете: condition? ? 42 : nil
В примера беше @name = if x[1] != nil then x[1].strip end
Това може да се запише така: @name = x[1] && x[1].strip

Грешка #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

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

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

Пише се string_tags, а не stringTags
Нещата в блокове се идентират с два интервала навътре.
Не три, не един, не нула. Два.
stringTags.has_key? (value) е психаделично. Пише се .has_key?(value).
За последното Ruby дори дава warning, но кой да чете.
n, arrArtist, @arrSongs, i са невероятно странни имена.
До тук, тази задача е на -2 точки.

Грешка #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

Ако блока е на повече от един ред се ползва do/end.
Скобата в if key == :tags е много странно затворена.
result = трябваше да е преди if-а

Източник

Грешка #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

Източник

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

Feature envy е един от code smell-овете в Refactoring.
Появява се, когато един клас много зависи от данните на друг.
Завижда му на feature-ите, демек.
В този случай, колекцията "завижда" на песента.
Решението е тези неща да се изместят от "завиждащия" клас в другия.
В случай, би се свело до song.matches? criteria.

Принцип: Single Responsibility Principle

Един от петте SOLID принципа.
Ще го дъвчем цяла година.
Гласи, че един клас трябва да прави точно едно нещо.
В нашия случай е адекватно колекцията да парси входа, а песента да прилага филтъра.
Така песента не зависи от входа (който се подава на колекцията).
Така колекцията не зависи само от един метод на песента - matches?.
Друга формулировка: един клас трябва да има точно една причина да се променя.

Shotgun surgery

code smell

Какво трябва да се промени във вашето решение, ако искаме да добавим нов филтър? Например по subgenre.
Очевидно песента.
Трябва ли да се промени Collection?
Shotgun surgery е когато една промяна изисква пипане в няколко различни класа.
Ако трябва да се пипате и Song и Collection, това е сипмтом.
Как може да организирате кода, така че да не се налага?
Може да напишете Collection така, че да не знае за полетата на Song.
Какво трябва да се промени, за да добавите нов атрибут на Song?
Тук подходящия дизайн не е толкова ясен.

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

Не са точна наука.
Не са и checklist-ове, на които кода ви да отговоря.
Добри инструменти за анализ.
Структурират ви мисловния процес, докато мислите върху дизайна.
По някога е ОК да не се съобразявате с тях. Понякога не е.
Въпрос на усет. Въпрос на опит.