Mailinglist Archive: yast-devel (246 mails)

< Previous Next >
Re: [yast-devel] Summary of code review
  • From: Klaus Kaempf <kkaempf@xxxxxxx>
  • Date: Mon, 24 Aug 2009 12:09:40 +0200
  • Message-id: <20090824100940.GB11052@xxxxxxxxxxxxx>
* Josef Reidinger <jreidinger@xxxxxxx> [Aug 18. 2009 15:52]:
Hi,
during today meeting with Karel (external rails consultant) we do some
public code review of my code and we also find some general problems. I
try summarize it in this mail. Please ask if you don't understand something.

Thanks for the review and your exhaustive mail !

I start with rest-service:
We should have some general method in ancestor (e.g.
YastApplicationController) of all webyast plugins which do something (so
no permissions or resources) some general method permission checker with
same behavior (like response permission denied).

The "More details to permission problems (high)" tasks targets in this
direction.

I guess this needs someone taking care for 'infrastructure' in
general. Any volunteers ?

We in general should eliminate things which is repeating in each
plugin.

:-) That's DRY (don't repeat yourself) in 'Ruby speak'.

Have one line view is useless and better is use respond_to.

Hmm, I moved a couple of respond_to's to views in the past to have
controller logic and resource representation more separated.

Any reasons why this is considered 'bad' ?



webclient:
My class LogException is useles if we disable backtrace silencing so
logger.info e is enought (btw it looks like our webclient is not
generated by rails 2.3, as few files missing, it looks like we generated
it by 2.2) http://afreshcup.com/2008/11/29/rails-23-backtrace-silencing/

Ah, interesting. How can we 'refresh' existing code to 2.3 ?

load_proxy method is good idea how to reduce needed lines of code but
should not redirect or set any infos. Instead use throwing exceptions
and use rescue_from to display correct page. This also discover why
sometime our universal exception handler doesn't work, because some
exception is not child of Exception but Error.

This sounds as we need more code checking tests. Any recommendations ?

This also discover how looks our Resource service and it is really
unreadable and unmaintainable code, as it is really big, should use e.g.
mixins or something else to reduce its size and complexity. Test for
this class looks good, so we should try refactor it to better result.

What is meant by 'Resource service' ?

We should better structure whole architecture like some part of
controller do things in one area which is not close to controller
contract, so we should move it to mixin (especially use it in
application controller).

I guess this is an ongoing refactoring effort we all have to spend
time on.

Any ideas how to mark such code ?

Karel doesn't like checking for gettext in application controller.

Why ? What does he suggest as an alternative ?


some positives from code review - time module is good documented and
testing looks good even if designer could recognize it is written after
code. Ideas to reduce code size and duplicated code is good, only
implementation is not always good.

some general knowledge:
- testing model is the easiest so move the most of functionality there
for easier testing
- over-mocking is not good, as it abstract from reality (it is useful
when you don't have implemented some functionality)
- .railsrc and .irbrc for some general routines available during
console session


I hope I don't forget much thinks, maybe someone add another knowledge.


It would be very helpful to move the general recommendations to a Wiki
page and/or add stuff to the ToDo list.

Klaus
---
SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG N├╝rnberg)

--
To unsubscribe, e-mail: yast-devel+unsubscribe@xxxxxxxxxxxx
For additional commands, e-mail: yast-devel+help@xxxxxxxxxxxx

< Previous Next >
Follow Ups
References