Refactoring #24

Closed
makaroni4 opened this Issue Apr 7, 2013 · 7 comments

Comments

Projects
None yet
2 participants

Hi guys!

How about a little refactoring? It is really hard to navigate and read flog source file, because it is basically one file 😃

https://codeclimate.com/github/seattlerb/flog

Owner

zenspider commented Apr 8, 2013

Internal refactorings are fine. Breaking up the file to no real effect doesn't make sense. Esp since that code climate score is partially based on flog. :P

zenspider was assigned Apr 8, 2013

Ryan, let me try to explain my point of view.

Lets recall Single Responsibility Principle. Class Flog deals with

  • plugins
  • folders and files
  • command line options
  • sexp processing
  • console output

I think it is a little bit overhead. If we move each logic part to its own class/module first thing any developer will notice when enter lib directory – "Ahaaaaa, flog has plugins, can analyse folders and files, has command line support and sexp configs and rate system." Is not it cool? To read through each method for ~1000 lines is difficult (for me at least). So what do you think about all that?

Nevertheless I don't insist on making some rearrangements (as you said right – veneer). Any refactoring implies test coverage first so I am focused on that. My next Pull Request will be mostly about tests (it is already in refactoring branch of my fork).

❤️ (one of the Github staff taught me to express respect in heart way 😃)

Owner

zenspider commented Apr 10, 2013

Don't get me wrong. The work you're doing is great. I just don't see much value in moving stuff out when, in fact, there isn't all that much of it. Looking at it:

823 lines incl comments

679 lines of code (and blanks) for Flog (82.5% of total)
 58 for consts, class vars, and attrs
 31 plugins (< 5% of LOC)
 32 cmdline processing (< 5% of LOC)
 78 options (11.5%... but a single method)
 47 flogging (these don't move, period)
239 lines devoted to processing nodes (ditto)
 87 output (12.8%)
107 misc (meh)

One thing I did notice is that I've already refactored myself into a bit of a mess. I need to move some of the methods around to make sure they're properly grouped. I'll do that after we merge your work in (to avoid conflicts).

Owner

zenspider commented Apr 10, 2013

OK. I got your pull merged. I'm gonna do a rearrangement of the methods. I hope it doesn't bork you.

Owner

zenspider commented Apr 10, 2013

For shits and giggles, I decided to refactor everything option, output, and cmdline related into FlogCLI.

FlogCLI is 225 LOC (incl comments)
Flog is now 628 LOC (incl comments). A 24% reduction from the original.

Honestly... I don't hate it. :P

I'm hesitant to commit because it will require test changes, which I know you're digging into right now.

Owner

zenspider commented Apr 10, 2013

OK. I must have hit the end of your day right about then. I'm gonna commit my refactoring & test changes. Sorry if this collides with your work. If you want, you can send me patches raw (unified diffs please) and I can merge them in. Emacs is really good at that. Git not so much.

Owner

zenspider commented Apr 11, 2013

I'm gonna mark this done... but keep going!

zenspider closed this Apr 11, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment