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@redhat.com>
To: alee@redhat.com
Cc: "Adam Young"<ayoung@redhat.com>, pki-devel@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@redhat.com>
To: pki-devel@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@redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel

_______________________________________________
Pki-devel mailing list
Pki-devel@redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel

_______________________________________________
Pki-devel mailing list
Pki-devel@redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel
_______________________________________________
Pki-devel mailing list
Pki-devel@redhat.com
https://www.redhat.com/mailman/listinfo/pki-devel