Pytanie o refactoring

Zakład patologii kodu
1

Chcę w przyszłym tygodniu zacząć szukać pracy jako programista RoR (nie wiem czy junior czy normalny, bo robiłem tylko hobbystyczne projekty). Właśnie ćwiczę sobie refaktorowanie kodu na takim przykładzie.

class Array
  def to_annotated_xml(root)
    output = "<#{root}>"

    each do |i|
      if i.is_a?(Fixnum)
        output << "<number>#{i}</number>"
      elsif i.is_a?(String)
        if i.match(/@/)
          output << "<email>#{i}</email>"
        else
          output << "<string>#{i}</string>"
        end
      else
        output << "<value>#{i}</value>"
      end
    end

    output << "</#{root}>"
  end
end

p [1, "Stephen", "stephenb@reallyenglish.com"].to_annotated_xml("person")

Na razie doszedłem do czegoś takiego: https://gist.github.com/mdoliwa/95bb0f4de8aaffefa93c087e77f6e264

Będę wdzięczny za komentarze, czy jest ok, co robię źle, czy to może przerost formy nad treścią i czy jest jakiś fajny patent na pozbycie się tego brzydkiego ifa z 71 linijki.

2

Wyglada na pierwszy rzut oka ok, wiadomo ze z refactorimngiem mozna poplynac i kwestia smaku.

Jesli chodzi o ifa, ja lubie myk z return

if value.is_a?(Fixnum)
  NumberXmlNode
elsif value.is_a?(String)
  value =~ /@/ ? EmailXmlNode : StringXmlNode
else
  XmlNode
end

zmienia sie na:

def klass
  return NumberXmlNode if value.is_a?(Fixnum)
  return ...
  XmlNode
end
3

W sumie właśnie wymyśliłem coś całkiem zgrabnego.
Usunąłem EmailXmlNode i ten przypadek obsługuję w StringXmlNode, tak, że jak się pojawi nowy Regexp to będzie wystarczało tam dopisać. Lekko poprawiłem XmlNodeFactory i w sumie ten if zniknął.

https://gist.github.com/mdoliwa/95bb0f4de8aaffefa93c087e77f6e264

4

Możecie pomóc rozwiązać takie zadanie :

Stwórz nowy hash o nazwie dictionary i przypisz mu wartość 5 pod klucz zdefiniowany w utworzonej przeze mnie zmiennej key. Następnie pod klucz 3 wpisz utworzoną przeze mnie zmienną value.

5

Ja z kolei lubię używać case’a zamiast if’ów.

class Array
  def to_annotated_xml(root)
    output = "<#{root}>"
    each do |i|
      output << case i
      when Fixnum
        "<number>#{i}</number>"
      when String
        i.match(/@/) ? "<email>#{i}</email>" : "<string>#{i}</string>"
      else
        "<value>#{i}</value>"
      end
    end
    output << "</#{root}>"
  end
end

EDIT:
zamiast tworzyć zmienną output, można też wykorzystać tap’a

6

Dodałeś nową funkcjonalność do Array, nie naruszyłeś logiki - rozszerzyłeś - nie widzę w tym nic zdrożnego

Masz prostą metodę parsującą, jaki jest powód refactoringu do drzewka obiektów ?

Dla podniesienia czytelności wystarczy dodać metodę “sugerującą” dla elementu tablicy typ XML

Bazując na Twoim pierwszym poście i gist ver. 1 … refactor

   class Array

     # wpisałem tę metodę tutaj bo nie chcę za długiego posta robić :)
     def which_xml_type(ruby_var)
       return 'number' if ruby_var.is_a?(Fixnum)
       return 'email' if ruby_var.is_a?(String) and ruby_var.match?(/@/)
       return 'string' if ruby_var.is_a?(String)
       return 'value'
     end

     def to_anotatated_xml(root)
       output = "<#{root}>"

       each do |i|
         xml_type = which_xml_type(i)
         output << "<#{xml_type}>#{i}</#{xml_type}>"
       end

       output << "</#{root}>"
     end
   end

metodę which_xml_type wypadało by sprywatyzować albo wyrzucić gdzieś na zewnątrz do modułu (typowa metoda “narzędziowa”)

Jeżeli traktować to jako zabawę z ruby - jak poprawić czytelność - to fajny przykład.

Jeżeli to ma być przyszły parser do XML to widzę schody nawet w moim rozwiązaniu :slight_smile:

W temacie refactoring, w poście metoda, więc odpuszczam obiektówkę - overkill

7

Naprawdę nic?
Przy takim podejściu kończymy z niewiadomą liczbą metod i przerośniętymi obiektami.

8

mówię o jednej metodzie

to_anotated_xml

nie widzę nic złego w tym żeby tablica potrafiła zrzucić się sama do XML

kwestia czego się używa by XML uzyskać

wiele gemów rozszerza standardowe klasy o swoje rzeczy

kolega spytał o refactoring metody, a nie o to czy dobrze zbudował mimikę XML dla przyszłego parsera

Osobiście wolę rozwiązanie z “samodzielną” tablicą

[1,2,3].to_anotated_xml(different_root_for_xml)

niż obiektówkę z opcjonalnym drugim parametrem dla innego root XML

MyParser.to_anotated_xml([1,2,3],different_root_for_xml)

Pytanie czy trzeba robić parser skoro już są całkiem sporo potrafiące

o ironio gdyby dodać do gemseta któryś z gotowych parserów metoda w Array była by mega krótka

Parsowanie XML to jest coś bardziej złożonego

http://xml4r.github.io/libxml-ruby/rdoc/index.html

Skoro @doli przymierza się do junior/mid to może wystarczy otestowany parser CSV zrobić

Jeśli to ma być parser pełną gębą to w wywołaniu oprócz tablicy przekazał bym xml schema

http://www.w3schools.com/xml/schema_intro.asp

gotowe parsery mają opcję wciśnięcia XML schema, wtedy “wczesna walidacja” daje możliwość rzucenia exceptiona zanim XML pójdzie dalej

Nie widzę sensu budowania parsera XML - są dobre, wydajne, gotowe

można użyć gotowca do prostej anotacji jak i do streamingu XML :slight_smile: gdyby był potrzebny

Co jest złego w gotowych parserach bazujących na libxml, że trzeba pisać własny ?

potraktowałem pytanie “as is” jako refactor metody a nie “czy ten parser da radę”

Jeżeli to ma działać z każdą tablicą i z każdym XML (nie tylko prosta anotacja w wąskim zakresie w ramach projektu) wtedy to co zaproponowałem jest bzdurą i nie ma wyjścia pozostaje “udekorować” gem do parsowania XML