6 ms·

A command line refactoring tool for PHP

62 points by beberlei 14y ago

tillk 14y ago
Looks neat. :) It would be pretty nice (if I may suggest that ;)), if you added add branch support so the patches are saved into a refactoring branch or maybe individual branches?
Egregore 14y ago
Can it be integrated into an ide? I like it a lot better when I point at method or variable in IDE and can quickly rename it. BTW: there is already such thing for PHP: PHPStorm, although not open source.
- beberlei 14y ago
  I suppose it can be integrated into IDEs. We (the developers) are Vim users, so there is no immediate need for us to integrate it into anything except Vim. We know that PHP Storm has many refactorings implemented very well, but a command line tool is much more flexible and useable for everyone, not just PHP Storm users.
binarymax 14y ago
I have minimal exposure to PHP, but recently helped a friend out with a small project on the weekends, that someone else he knew had built. When I first saw the codebase I found a veritable mess of very unreadable unmaintainable code. No functions - no OOP of any kind - no separation of concerns - nothing reusable - SQL calls (probably vulnerable to injection) mixed with HTML - a hulking mess of spaghetti. Can tools like these fix code like that, or at least help? It would have taken me about a day per page just to make it readable - and there were dozens of pages. I don't plan on going back into that minefield if I can help it, but it would be good to know in the future. --EDIT-- Thanks for all the helpful replies. Luckily I'm pretty sure the project is behind me. Seeing that PHP reminded me of old bad ASP (pre .NET) - it was easy to get away with lots of really bad stuff (on error resume next anyone?). It makes me wonder if auto-refactoring spaghetti is a crackable problem. When I dared look I did see lots of things that I could have fixed with a little bash and sed, but of course I am human so I understand the 'intent' of the code. Fascinating stuff.
- ohwp 14y ago
  Those problems are not related to PHP but to inexperienced developers. Refactoring such code is always a pain. So unless there is nice money involved: don't refactor, just fix what's broken. I don't think refactoring of spaghetti can be automated. But there are other tools that can help you to find bad code. Some that come in mind: https://github.com/fabpot/PHP-CS-Fixer http://pear.php.net/package/PHP_CodeSniffer/redirected Those tools are used for coding standards (PSR-x), but those standards are also helpful to track down bad code.
  - itafroma 14y ago
    Good advice, with a couple of caveats about PSRs: - They're not all about coding standards: only two of them are. PSR-0 is a class autoloading standard and PSR-3 is a common interface for loggers. The next PSR is likely going to be a common caching interface. - PSR-1 and PSR-2 are controversial (although PSR-1 less so than PSR-2), and don't necessarily represent best practices amongst the larger PHP community. They're useful insofar as the PHP Framework Interoperability Group (PHP-FIG, http://www.php-fig.org/ http://www.php-fig.org/), the people crafting the PSRs, needed an internal standard and really shouldn't be represented as anything more than that. Much of both standards are an amalgam of individual practices produced via an internal survey of PHP-FIG member projects. If you have nothing, PSR-1 and PSR-2 aren't bad places to start, but diverging from them is more likely to happen than not, especially if you're working with projects or teams that have existing coding standards.
    - h2s 14y ago
      > PSR-1 and PSR-2 are controversial > Much of both standards are an amalgam of individual > practices produced via an internal survey of PHP-FIG > member projects. Those member projects include codebases like Zend Framework, Symfony, Composer and Doctrine. Those standards are are a very accurate representation of the way the best, most popular open source PHP code is being written. The only controversy I'm aware of is those petty pull requests opened by people who didn't like that they chose spaces over tabs, which eventually drove one of the founders of the group to quit out of frustration.
itry 14y ago
Is this really needed? I get away with :%s/$abcd/$this->abcd/gc just fine. When you leave vim and do this: $ php refactor.phar convert-local-to-instance-variable tests/FooServiceTest.php\ 6 dependency | patch -p1 I stay in vim and simply do this: :%s/$dependency/$this->dependency/gc And hit y a couple of times. Pros: I dont need to install, trust and learn a new tool. I dont have to worry if i broke somthing. Because of edge cases the tool did not handle or because I messed up the syntax.
- beberlei 14y ago
  You picked the simplest refactoring, for converting local to instance variable I agree with you, this is easy in Vim. But still there are some potential problems: 1. The variable already exists in the class. 2. You accidently mark to much or to few lines with ":<,>%" For refactorings such as extract method the number of preconditions and information to gather is much bigger, such as input and output variables, existance of return statement etc. We plan to integrate the tool into vim as well, so that you can call the refactorings through vim directly.
  - itry 14y ago
    I did not pick anything. It is your example. Right from your page. And the only one you provide. If you have a better one: show us.
    - beberlei 14y ago
      The blog post shows extract method as well as a second refactoring Edit: Also see the screenshot here for a more complex extract method refactoring: http://qafoolabs.github.io/php-refactoring-browser/ http://qafoolabs.github.io/php-refactoring-browser/
      - itry 14y ago
        Ah, I missed the extract-method example. Similarely to the convert-local-to-instance-variable example, to me it looks like overkill. I never had a problem, doing these things manually. But if you like doing it this way - I dont complain. To each his own.
Edd314159 14y ago
Seems like a neat and very clever tool, but why would you use a test case as an example? Maintainability in unit tests is nowhere near as important as with actual implementation code (refactoring eventually may even hinder the documentary aspect of test code).
- h2s 14y ago
  I respectfully disagree. If you don't treat test code with the same respect as production code, rot sets in. Rot in the test suite discourages programmers from trusting, augmenting and maintaining it, until it eventually stops being used altogether. The most common type of rot in test code is caused by lazily copy/pasting vast blocks of setup code. The reason this is bad is that when subtle architectural changes break that setup process, somebody has to waste hours fixing 50 tests. Refactoring such code into a setUp method is the usual way of avoiding this.
- beberlei 14y ago
  I disagree heavily. Refactoring tests is as important as refactoring "production" code. Following the four elements of simple design (http://www.jbrains.ca/permalink/the-four-elements-of-simple-design http://www.jbrains.ca/permalink/the-four-elements-of-simple-...) you can use test refactorings to find better names and detect need for abstractions. Tests with lots of duplication, for example in the test setup, get hard to maintain and developers tend to abandon them more easily (my own experience).
- wilmoore 14y ago
  Unfortunately, this is a very common mistake made by those that treat their tests a an after-thought. Tests should be cared for and maintained as much as (or perhaps even more than) the production code. If tests are difficult to read or reason about, it is much more likely that they will become stale, brittle, or worse, be ignored. If one does not plan to keep tests well-maintained, might as well never add them in the first place.
moron4hire 14y ago
Yeah, but then you're still working in PHP. I cannot state clearly enough, PHP will not help you. Neither in your project or in your career. If you are doing PHP, do everything in your power to switch to something else. There is no use case for PHP that is not better served by something else.

62 points by beberlei 14y ago

tillk 14y ago

Looks neat. :) It would be pretty nice (if I may suggest that ;)), if you added add branch support so the patches are saved into a refactoring branch or maybe individual branches?

Egregore 14y ago

Can it be integrated into an ide? I like it a lot better when I point at method or variable in IDE and can quickly rename it. BTW: there is already such thing for PHP: PHPStorm, although not open source.

beberlei 14y ago
I suppose it can be integrated into IDEs. We (the developers) are Vim users, so there is no immediate need for us to integrate it into anything except Vim. We know that PHP Storm has many refactorings implemented very well, but a command line tool is much more flexible and useable for everyone, not just PHP Storm users.

binarymax 14y ago

I have minimal exposure to PHP, but recently helped a friend out with a small project on the weekends, that someone else he knew had built. When I first saw the codebase I found a veritable mess of very unreadable unmaintainable code. No functions - no OOP of any kind - no separation of concerns - nothing reusable - SQL calls (probably vulnerable to injection) mixed with HTML - a hulking mess of spaghetti. Can tools like these fix code like that, or at least help? It would have taken me about a day per page just to make it readable - and there were dozens of pages. I don't plan on going back into that minefield if I can help it, but it would be good to know in the future. --EDIT-- Thanks for all the helpful replies. Luckily I'm pretty sure the project is behind me. Seeing that PHP reminded me of old bad ASP (pre .NET) - it was easy to get away with lots of really bad stuff (on error resume next anyone?). It makes me wonder if auto-refactoring spaghetti is a crackable problem. When I dared look I did see lots of things that I could have fixed with a little bash and sed, but of course I am human so I understand the 'intent' of the code. Fascinating stuff.

ohwp 14y ago
Those problems are not related to PHP but to inexperienced developers. Refactoring such code is always a pain. So unless there is nice money involved: don't refactor, just fix what's broken. I don't think refactoring of spaghetti can be automated. But there are other tools that can help you to find bad code. Some that come in mind: https://github.com/fabpot/PHP-CS-Fixer http://pear.php.net/package/PHP_CodeSniffer/redirected Those tools are used for coding standards (PSR-x), but those standards are also helpful to track down bad code.
- itafroma 14y ago
  Good advice, with a couple of caveats about PSRs: - They're not all about coding standards: only two of them are. PSR-0 is a class autoloading standard and PSR-3 is a common interface for loggers. The next PSR is likely going to be a common caching interface. - PSR-1 and PSR-2 are controversial (although PSR-1 less so than PSR-2), and don't necessarily represent best practices amongst the larger PHP community. They're useful insofar as the PHP Framework Interoperability Group (PHP-FIG, http://www.php-fig.org/ http://www.php-fig.org/), the people crafting the PSRs, needed an internal standard and really shouldn't be represented as anything more than that. Much of both standards are an amalgam of individual practices produced via an internal survey of PHP-FIG member projects. If you have nothing, PSR-1 and PSR-2 aren't bad places to start, but diverging from them is more likely to happen than not, especially if you're working with projects or teams that have existing coding standards.
  - h2s 14y ago
    > PSR-1 and PSR-2 are controversial > Much of both standards are an amalgam of individual > practices produced via an internal survey of PHP-FIG > member projects. Those member projects include codebases like Zend Framework, Symfony, Composer and Doctrine. Those standards are are a very accurate representation of the way the best, most popular open source PHP code is being written. The only controversy I'm aware of is those petty pull requests opened by people who didn't like that they chose spaces over tabs, which eventually drove one of the founders of the group to quit out of frustration.

itry 14y ago

Is this really needed? I get away with :%s/$abcd/$this->abcd/gc just fine. When you leave vim and do this: $ php refactor.phar convert-local-to-instance-variable tests/FooServiceTest.php\ 6 dependency | patch -p1 I stay in vim and simply do this: :%s/$dependency/$this->dependency/gc And hit y a couple of times. Pros: I dont need to install, trust and learn a new tool. I dont have to worry if i broke somthing. Because of edge cases the tool did not handle or because I messed up the syntax.

beberlei 14y ago
You picked the simplest refactoring, for converting local to instance variable I agree with you, this is easy in Vim. But still there are some potential problems: 1. The variable already exists in the class. 2. You accidently mark to much or to few lines with ":<,>%" For refactorings such as extract method the number of preconditions and information to gather is much bigger, such as input and output variables, existance of return statement etc. We plan to integrate the tool into vim as well, so that you can call the refactorings through vim directly.
- itry 14y ago
  I did not pick anything. It is your example. Right from your page. And the only one you provide. If you have a better one: show us.
  - beberlei 14y ago
    The blog post shows extract method as well as a second refactoring Edit: Also see the screenshot here for a more complex extract method refactoring: http://qafoolabs.github.io/php-refactoring-browser/ http://qafoolabs.github.io/php-refactoring-browser/
    - itry 14y ago
      Ah, I missed the extract-method example. Similarely to the convert-local-to-instance-variable example, to me it looks like overkill. I never had a problem, doing these things manually. But if you like doing it this way - I dont complain. To each his own.

Edd314159 14y ago

Seems like a neat and very clever tool, but why would you use a test case as an example? Maintainability in unit tests is nowhere near as important as with actual implementation code (refactoring eventually may even hinder the documentary aspect of test code).

h2s 14y ago
I respectfully disagree. If you don't treat test code with the same respect as production code, rot sets in. Rot in the test suite discourages programmers from trusting, augmenting and maintaining it, until it eventually stops being used altogether. The most common type of rot in test code is caused by lazily copy/pasting vast blocks of setup code. The reason this is bad is that when subtle architectural changes break that setup process, somebody has to waste hours fixing 50 tests. Refactoring such code into a setUp method is the usual way of avoiding this.
beberlei 14y ago
I disagree heavily. Refactoring tests is as important as refactoring "production" code. Following the four elements of simple design (http://www.jbrains.ca/permalink/the-four-elements-of-simple-design http://www.jbrains.ca/permalink/the-four-elements-of-simple-...) you can use test refactorings to find better names and detect need for abstractions. Tests with lots of duplication, for example in the test setup, get hard to maintain and developers tend to abandon them more easily (my own experience).
wilmoore 14y ago
Unfortunately, this is a very common mistake made by those that treat their tests a an after-thought. Tests should be cared for and maintained as much as (or perhaps even more than) the production code. If tests are difficult to read or reason about, it is much more likely that they will become stale, brittle, or worse, be ignored. If one does not plan to keep tests well-maintained, might as well never add them in the first place.

moron4hire 14y ago

Yeah, but then you're still working in PHP. I cannot state clearly enough, PHP will not help you. Neither in your project or in your career. If you are doing PHP, do everything in your power to switch to something else. There is no use case for PHP that is not better served by something else.