openlp-core team mailing list archive

[jerryb <gerald.britton@xxxxxxxxx>]

On Mon, May 9, 2011 at 4:37 AM, Jonathan Corwin <j@xxxxxxxxxxxx> wrote:
> Review: Approve
> Well I'm going to approve this because Raoul has and my only objection was standards :)

As I noted, I followed the standards with this patch and took
advantage of the blank line exception clause to improve the overall
readability of the module.

>
> However I'm still not overly happy about the blank lines, please do not see this approval to give you free reign to add them everywhere, else we'll just end up with a blank line war where some people are adding them and others are removing them. Thanks.

That would be entertaining to see!  Seriously though, a blanket "no
blank lines" policy is bad policy in my opinion and can lead to quite
unreadable (and thus unmaintainable) code.  I have had to struggle
through (and fix) modules like that.  Moderation should be the rule.
Frame logical sections of code with appropriate comments and the
judicious use of vertical whitespace.

Far more objectionable in my view are deep indentation levels.  I have
had to correct modules where the indentation was so deep (many levels
of loops and ifs) that there was only negligible space before the
standard 72-column limit (PEP 8, though I notice OpenLP relaxes that
somewhat.)  I would recommend a limit on indentation to some
agreed-upon depth such that after "n" levels a programmer must split a
function into two or more sub-functions.  This also improves code
readability.

Note: I'm copying the mailing list for further discussion regarding
indentation levels.


> --
> https://code.launchpad.net/~gerald-britton/openlp/bugs/+merge/60225
> You are the owner of lp:~gerald-britton/openlp/bugs.
>



-- 
Gerald Britton

https://code.launchpad.net/~gerald-britton/openlp/bugs/+merge/60225
Your team OpenLP Core is subscribed to branch lp:openlp.