On Thu, 31 Jul 2014 10:21:35 +0200 Arvin Schnell wrote:

On Wed, Jul 30, 2014 at 03:32:50PM +0200, Josef Reidinger wrote:

...

I found this nice article[1] that nicely and visible summarize all my arguments why refactoring should not be separated tasks, but integral part of all work. I think it deserve reading.

In general I agree with the article but it doesn't apply to YaST since we already have years of backlog for refactoring. So for us making it correct does not take a "little bit longer" but likely several times as long. Also doing a bit refactoring always has the risk of regressions and without unit tests *and* integration tests these are hard to discover so often I just do not dare.

Regards, Arvin

In this case it is not so easy, but there is ways. E.g. one of possible ways is to write test as part of change, then refactor as you are more brave to do it. There is also parts which is very hard to test as it have many side-effects or keep states. I think in such situation you can do minimal modification to make it more testable, write test that proves it and then refactor it more deeply. e.g. in this pull request - https://github.com/yast/yast-bootloader/pull/127 I at first modify changeOrderInDeviceMapping to not modify directly @device_mapping, but have it as parameter, write tests for it and then add functionality I need with additional tests. In general any change that improve isolation of method greatly helps with testing of it. Another think I am trying to do, is to break it more in earlier phases ( like when I implement features as my impression is that each feature contain at least one bug :), so I made bigger changes and were more brave, writed tests for it and when some regressions apeared, then I improve tests to prevent it and also can refactor more aggressive this new code when it is needed. Josef -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

On 31.7.2014 10:33, Josef Reidinger wrote:

On Thu, 31 Jul 2014 10:21:35 +0200 Arvin Schnell wrote:

...

In general I agree with the article but it doesn't apply to YaST since we already have years of backlog for refactoring. So for us making it correct does not take a "little bit longer" but likely several times as long. Also doing a bit refactoring always has the risk of regressions and without unit tests *and* integration tests these are hard to discover so often I just do not dare.

In this case it is not so easy, but there is ways. E.g. one of possible ways is to write test as part of change, then refactor as you are more brave to do it. There is also parts which is very hard to test as it have many side-effects or keep states. I think in such situation you can do minimal modification to make it more testable, write test that proves it and then refactor it more deeply. e.g. in this pull request - https://github.com/yast/yast-bootloader/pull/127 I at first modify changeOrderInDeviceMapping to not modify directly @device_mapping, but have it as parameter, write tests for it and then add functionality I need with additional tests. In general any change that improve isolation of method greatly helps with testing of it. Another think I am trying to do, is to break it more in earlier phases ( like when I implement features as my impression is that each feature contain at least one bug :), so I made bigger changes and were more brave, writed tests for it and when some regressions apeared, then I improve tests to prevent it and also can refactor more aggressive this new code when it is needed.

We can also look at it from the other point of view: What do we lose if we actually do not refactor anything? Obviously, we will lose the possibility to implement anything easily in the future or fix things properly. Anytime we add something, we make the code a bit less understandable and a bit easier to break. Especially with years of coding and fixing, most of the code already looks like spaghetti. The code is more and more complex and cleaning it is vitally important to our own health. It's similar to cooking: Anytime you want to cook something, you need to cleanup the kitchen desk first, wash the dishes etc. Oh, sure, even better is to do it while cooking or after it's finished. Now imagine that you come to a messy kitchen where everything is dirty and at strange places and you have to make something very good and very fast. Almost impossible. So, from this POV, refactoring (most probably using smaller steps) is the only way to go - obviously while writing tests for everything. Nobody can see the future, nobody can tell what will happen tomorrow, but we can be almost sure that there will be bugs to fix and features to implement in Yast. Bye Lukas -- Lukas Ocilka, Systems Management (Yast) Team Leader Cloud & Systems Management Department, SUSE Linux -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org

On Thu, 31 Jul 2014 11:07:51 +0200 Arvin Schnell wrote:

On Thu, Jul 31, 2014 at 10:33:02AM +0200, Josef Reidinger wrote:

...

In this case it is not so easy, but there is ways. E.g. one of possible ways is to write test as part of change, then refactor as you are more brave to do it. There is also parts which is very hard to test as it have many side-effects or keep states. I think in such situation you can do minimal modification to make it more testable, write test that proves it and then refactor it more deeply. e.g. in this pull request - https://github.com/yast/yast-bootloader/pull/127 I at first modify changeOrderInDeviceMapping to not modify directly @device_mapping, but have it as parameter, write tests for it and then add functionality I need with additional tests. In general any change that improve isolation of method greatly helps with testing of it. Another think I am trying to do, is to break it more in earlier phases ( like when I implement features as my impression is that each feature contain at least one bug :), so I made bigger changes and were more brave, writed tests for it and when some regressions apeared, then I improve tests to prevent it and also can refactor more aggressive this new code when it is needed.

I could also give examples where I refactored something during feature development but I can also give examples where it did not work:

- Last year I tried to make some dialog in yast2-storage object oriented. As discussed on this list with the strange ruby mixin a major rewrite would have been required.

I agree that for some parts it is more tricky then for others. In general more coupled ( http://en.wikipedia.org/wiki/Coupling_%28computer_programming%29 ) code tent to be hard to change and also test isolated. One possible strategy how to deal with it, is find the least coupled part and move it out, as it should be relative easy and decrease coupling of remaining modules depending on it. Some articles with hints how to decrease coupling: - http://www.codeodor.com/index.cfm/2009/6/17/Strive-for-low-coupling-and-high... - http://www.informit.com/articles/article.aspx?p=1946176&seqNum=2 (quite nice one and with named parameters, it can look even better) - http://martinfowler.com/ieeeSoftware/coupling.pdf - http://alancohen.tumblr.com/page/2 - http://www.jasoncoffin.com/cohesion-and-coupling-principles-of-orthogonal-ob... ( especially this one really show some problems of YaST )

- One bug requests a small partition (within the size of cylinders). Since YaST calculates in cylinders this is problematic. Changes will require modification to various modules (e.g. libstorage, yast2-storage, yast2-bootloader, likely also AutoYaST). Didn't happen due to no time at all - at least on my side.

Well, this is not refactoring, but design change. I worry, there is no easy way to change it if all modules takes it as axiom. What you can do to prevent it in future is abstraction which allows easy change of underlaying units. In fact e.g. bootloader usually do not care if sizes are in cylinders or bytes, only in very specific cases when there is limit in bootloader itself.

So I still have not seen a reasonable concept how to improve the situation with YaST.

I hope we will discuss it on workshop with conclusion what metrics we will use and what need modified code fulfill ( e.g. if we have metrics and marks, then we can say, if you touch this file or method, then final file or method must have B grade in feature phase, C in Beta phase and D in RC phase ). There is already some discussions about it on this mailing list, so I thing we just need to decide what we will use.

One thing I consider a requirement are automated integration test as discussed before. But half a year later still no progress.

There is some, we have at least autoyast tests in openqa.suse.de. Of course we can do more, but at least we start with it.

Regards, Arvin

Josef -- To unsubscribe, e-mail: yast-devel+unsubscribe@opensuse.org To contact the owner, e-mail: yast-devel+owner@opensuse.org