[Pki-devel] Automatic reformatting and code style
Nathan Kinder
nkinder at redhat.com
Tue Nov 29 02:27:28 UTC 2011
On 11/28/2011 05:40 PM, Christina wrote:
> I agree with Ade.
>
> I have been busy with the actual pki implementation and bug fixes and
> I'm not sure if I'm supposed to pay attention to this subject (and I
> didn't much), but I think it's worthwhile to chime in. I have worked
> on the project for maybe 13 or 15 years since my Netscape days, so I
> may be partial, but I think it's a good idea to hear out both sides of
> the debate.
>
> This is not the 1st controversial item that I have observed, and I
> have no idea what happened to the other ones in the end.
> May I suggest that you guys separate the non-controversial items from
> the controversial ones and leave the controversial ones alone until
> people debate it out? Perhaps a wiki page to capture all the
> controversial issues so for those of us who can't follow the whole
> email threads all the time can have a chance to go to the wiki and
> find out points from both sides of the debate?
> Each side should be able to go to the wiki and put down your points.
> Then we vote at some point, hopefully when I'm around?
>
> Is this acceptable?
I believe that some of these decisions need to be made now, as we only
want to be making large refactoring changes once. The longer we drag it
out, the more painful it is going to be to work on the code as things
will constantly be in flux.
I can see Ade's point about not having an easy way to clean up all of
the "mFoo" type variables automatically, but do we really want to
require all new code to use the "m" prefix? That will cause you to have
to change the variable name in a potentially large number of places if
one decides to change the scope of a variable, which is really just
extra work that shouldn't be needed. The other thing I don't like about
encoding the scope into the variable name is that it might be
incorrect. Just because a variable is named "mFoo" doesn't mean it's
really a member variable. This can lead to false assumptions when one
should really consult the declaration to see what it truly is. The
ability for the naming to be inconsistent with the declaration makes
encoding the scope useless (and potentially bad) IMHO. I know I've been
very frustrated running into variable names in C that had the type
encoded originally, but someone changed the type in the declaration
without changing the variable name.
>
> thanks!
> Christina
>
> On 11/28/2011 07:34 AM, Ade Lee wrote:
>> I had no idea that the naming of interfaces and member fields would
>> generate such spirited debate.
>>
>> I'm going to play devil's advocate here, and argue FOR keeping the
>> convention of prefixing "m" to object fields.
>>
>> 1. The arguments against Hungarian notation have mostly been made
>> against what is called "systems Hungarian" - that is, things that are
>> readily checked by a compiler or an IDE.
>>
>> When I hover over a variable in eclipse, it will tell me the type - so
>> there is no need to prefix the variable with that information. Using
>> iFoo for an integer is counterproductive for those reasons.
>>
>> Attaching "m" to an object tells us about scope - which is not displayed
>> when I hover over the variable. Sure, I can look up the declaration -
>> but with the prefix, I don't have to.
>>
>> 2. Does the "m" prefix make the code less readable? I don't think so -
>> we're not talking about plzFoo here. What it does do, is allow me when
>> looking at segment of code - to instantly determine whether the
>> variables in the code are local to the function or member variables.
>>
>> 3. You may want to change the scope of a variable - and this would
>> require you to rename the variable. But I would argue that this is a
>> conscious decision - and one that will likely require the
>> addition/removal of getters/setters. Having the variable prefixed by
>> "m" requires you to think about that. And as has been mentioned,
>> renaming is trivial in eclipse.
>>
>> 4. Renaming a specific variable is trivial in eclipse, but I have not
>> found a simple way to do a mass renaming so that all the variables that
>> are now in Hungarian notation in eclipse. There is a way to enforce the
>> use of a prefix - but not a way to systematically remove the prefixes.
>>
>> If we can't do that, then we run the risk of having code which conformed
>> to old standard - along with code which conformed to a new standard
>> which did not require the "m" - which I would argue is much worse.
>>
>> 5. Even if we could remove all Hungarian notation, this would make
>> merges from 8.X to the tip much more labor intensive. Its one thing to
>> have to do things manually because of formatting. Its another to have
>> to replace variables stuck in the code.
>>
>> Ade
>>
>> On Wed, 2011-11-23 at 17:50 -0500, Adam Young wrote:
>>> vi long since became capable of handling either format. I'll leave
>>> it as ana excersize to you vi guys to figure out how to get it to
>>> match.
>>>
>>> Renaming is trivial in Eclipse, and "Its always been done this
>>> way" is not sufficient grounds to keep up a bad practice.
>>>
>>>
>>> The code is going to shrink, get reordred, and things are going to
>>> get renamed. THis is acleanup that will have a very large net
>>> positive advantage, and we have the flexibility to do it right.
>>> Lets not hamstring outselves with decisions bad in the past that no
>>> longer apply: keep the good, but don't hoard things, esepcially
>>> not in code.
>>>
>>> INterfaces have their uses, but the PKI code base shows some of the
>>> mistakes made by following the "best practices" of the day that
>>> have since been discarded. Interfaces in Java should be used for
>>> cutting dependncies, information hading and the like, but should
>>> be the exception, not the rule. A fundamental guideline is "favor
>>> collaboration over inheritance" and that means Interfaces as well
>>> as abstract base classes. I'm not saying "Don't use them" but I
>>> am saying "Be prepared to justify their use" if you do.
>>>
>>>
>>> For the domain model, using an IRequest to front RequestImpl where
>>> there is only one Impl is an anti-pattern. a Date transfer object
>>> should have minimal functionality in it, and no external
>>> dependencies. Thus, it gains no benefit from the abstraction of
>>> the Interface.
>>>
>>> A much better approach is to make Request a POJO, and, if
>>> posssible, immutable. I'll go more into how to code this way as
>>> examples pop up.
>>>
>>>
>>>
>>> Sorry for top posting, but I'm having mail failure and can only
>>> use Zimbra web, which doesn't do the> thing.
>>>
>>>
>>> ----- Original Message -----
>>> From: "Matthew Harmsen"<mharmsen at redhat.com>
>>> To: alee at redhat.com
>>> Cc: "Adam Young"<ayoung at redhat.com>, pki-devel at redhat.com
>>> Sent: Wednesday, November 23, 2011 5:05:47 PM
>>> Subject: Re: [Pki-devel] Automatic reformatting and code style
>>>
>>> On 11/23/11 11:57, Ade Lee wrote:
>>>> I looked at our current guidelines and noticed the following
>>>> differences
>>>> with the Sun guides:
>>>>
>>>> 1. Placement of braces
>>>> In the current coding guidelines, we say classes and methods should
>>>> look
>>>> like this:
>>>> public class MyClass
>>>> {
>>>> ...
>>>> }
>>>>
>>>> instead of this (as in the Sun standard):
>>>> public class MyClass {
>>>> ...
>>>> }
>>> The history behind this probably comes from a nice handy 'vi' shortcut
>>> (pressing ']' twice) that allows for developers to quickly find the
>>> beginning of
>>> various 'functions' in 'C'/'C++'; note that the opening brace must be a
>>> standalone
>>> character in the first column for this to work.
>>>
>>> This was probably applied to the Java code to find the beginning and
>>> ending of a
>>> 'class' in java files that contained more than one non-nested class
>>> in a
>>> single '.java'
>>> file, so I don't see that big of a problem in changing this for Java --
>>> however,
>>> I would prefer that 'C'/'C++' functions/methods retain this handy
>>> feature, as
>>> not every programmer is particularly fond of IDEs, and I would argue
>>> that we
>>> should not require developers to use an IDE such as eclipse.
>>>
>>> That being said, I would prefer multiple non-nested public classes to
>>> reside in their
>>> own files, leaving only cases where we need/require standalone
>>> private/protected
>>> classes to co-exist within the same file? I am uncertain if we even
>>> have any of these.
>>>
>>>> 2. We require that interface names begin with "I". Sun says nothing
>>>> about this. This is probably a good one to keep.
>>> I agree that we should keep this 'handy' notation.
>>>> 3. We specify "no tabs". Sun says tabs are optional. We should keep
>>>> this.
>>> My personal preference is to use 'spaces' instead of 'tabs' primarily
>>> due to the
>>> variable 'tabstop' settings in files.
>>>> 4. We say "Static methods should begin with a capital letter with each
>>>> subsequent new word in uppercase, and subsequent letters in each
>>>> word in
>>>> lower case. Sun has no special treatment for static methods - ie.
>>>> they
>>>> are treated just like other methods .. ie. beginning with a lower-case
>>>> letter.
>>> I have no particular preference one way or another on this.
>>>> 5. We do have some guidelines for naming functions that go beyond what
>>>> Sun specifies - and also perhaps, beyond what eclipse can verify.
>>>>
>>>> For example:
>>>> * Get and set methods should begin with "get" / "set" and return the
>>>> appropriate object type.
>>>> * Boolean get methods should use "is" or "can" as a prefix, such as
>>>> "isUndoable()" rather than "getUndoable()".
>>>> * Factory class names should include the word "Factory". Factory
>>>> method
>>>> names should start with the word "Make."
>>>> * Methods for debug-only implementations should begin with "debug".
>>>> * Member variables should begin with "m". For example,
>>>> mMemberVariable.
>>> These all seem fine to me.
>>>
>>> While I understand Adam's doesn't like Hungarian notation, the
>>> downside of renaming everything will make it far more difficult
>>> for the people who are the most familiar with this code to
>>> continue to make changes.
>>>
>>>> My take on this is that we should adopt the Sun coding standards
>>>> and add
>>>> the additional requirements that make sense - like the ones listed in
>>>> point 5 above. For the cases where we conflict with the Sun
>>>> standards,
>>>> we should go with the Sun standards instead.
>>>>
>>>> Comments?
>>>> Ade
>>>>
>>>> On Wed, 2011-11-23 at 12:26 -0500, Adam Young wrote:
>>>>> MIght I highly encourage that we folow the Sun guides, as it is
>>>>> the Inustry standard in Java, and it is pretty staightforward.
>>>>>
>>>>> ----- Original Message -----
>>>>> From: "Ade Lee"<alee at redhat.com>
>>>>> To: pki-devel at redhat.com
>>>>> Sent: Wednesday, November 23, 2011 10:48:42 AM
>>>>> Subject: [Pki-devel] Automatic reformatting and code style
>>>>>
>>>>> Hi all,
>>>>>
>>>>> It has been decided that the code should go through an automatic
>>>>> reformatting on the trunk to ensure that everything matches the
>>>>> project's coding standards.
>>>>>
>>>>> Prior to this, we need to review the coding standards and confirm
>>>>> that
>>>>> they are what we want to use.
>>>>>
>>>>> The current coding standards for the project are referenced here:
>>>>> http://pki.fedoraproject.org/wiki/PKI_C_Coding_Style
>>>>> http://pki.fedoraproject.org/wiki/PKI_Java_Coding_Style
>>>>>
>>>>> Some alternative styles:
>>>>> http://freeipa.org/page/Coding_Style (C)
>>>>> http://www.oracle.com/technetwork/java/codeconvtoc-136057.html (java,
>>>>> sun conventions)
>>>>>
>>>>> We should focus on the java coding style first, followed by C.
>>>>> Most of
>>>>> the Perl code is mostly going away most likely, so no need to
>>>>> focus on
>>>>> that.
>>>>>
>>>>> IPA has a style guide for python, which, unless we have another
>>>>> compelling reason, we should probably use that:
>>>>>
>>>>> http://freeipa.org/page/Python_Coding_Style
>>>>>
>>>>> We'd like to get this resolved soon - so as not to obscure any future
>>>>> changes as we do new development. So, please devote some
>>>>> attention to
>>>>> this soon.
>>>>>
>>>>> Thanks,
>>>>> Ade
>>>>>
>>>>> _______________________________________________
>>>>> Pki-devel mailing list
>>>>> Pki-devel at redhat.com
>>>>> https://www.redhat.com/mailman/listinfo/pki-devel
>>>> _______________________________________________
>>>> Pki-devel mailing list
>>>> Pki-devel at redhat.com
>>>> https://www.redhat.com/mailman/listinfo/pki-devel
>>
>> _______________________________________________
>> Pki-devel mailing list
>> Pki-devel at redhat.com
>> https://www.redhat.com/mailman/listinfo/pki-devel
>
>
>
>
> _______________________________________________
> Pki-devel mailing list
> Pki-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/pki-devel/attachments/20111128/1fa90ef8/attachment.htm>
More information about the Pki-devel
mailing list