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
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
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 { ... } 2. We require that interface names begin with "I". Sun says nothing about this. This is probably a good one to keep. 3. We specify "no tabs". Sun says tabs are optional. We should keep this. 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. 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. 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
On 11/23/2011 1:57 PM, Ade Lee wrote:
1. Placement of braces
I'd say we should go with Sun's style, it's more common and would be consistent with the rest of the code (e.g. if/while/switch/try):
public class MyClass { ... }
2. We require that interface names begin with "I". Sun says nothing about this. This is probably a good one to keep.
Yes.
3. We specify "no tabs". Sun says tabs are optional. We should keep this.
Yes.
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.
Yes, I don't think it needs any special treatment. Most IDE will show it italicized.
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.
OK.
* Boolean get methods should use "is" or "can" as a prefix, such as "isUndoable()" rather than "getUndoable()".
There are probably some other words too, e.g. 'has'.
* Factory class names should include the word "Factory". Factory method names should start with the word "Make."
Would 'create' be better? 'make' doesn't always mean creating a new object.
* Methods for debug-only implementations should begin with "debug".
OK.
* Member variables should begin with "m". For example, mMemberVariable.
I'd prefer not to use any prefix because sometimes we want to expose an attribute publicly, in that case it would be better to use less cryptic names. Most IDE will color it differently too. -- Endi S. Dewata
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 { ... }
Yes please;
2. We require that interface names begin with "I". Sun says nothing about this. This is probably a good one to keep.
NO! This should go away. We definitely have too many interfaces. If there is only one Impl, we should merge Impl and interface. Hungarian notation is a crimae against Humanity. The interface should get the Good, simple name, and the Impl sould get whatever makes it special.
3. We specify "no tabs". Sun says tabs are optional. We should keep this.
Agreed. No tabs
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.
lowercase the first letter. It is what everyone expects.
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".
These are all fine. get/set is the bean API. While I am not a fan, doing this does make things work with automated tools.
* Member variables should begin with "m". For example, mMemberVariable..
'm' should go away. Again, hungarina notation, obfusticates the code, and makes it hard to read.
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.
Agreed. I'd like to exorcise the Hungarian naming conventions, but that can be done over time. Code is meant to be readable, and all HN does is obfuscate. It is not necessary in Java. I am pretty sure it is explictly against the Sun Java Conventions. http://www.oracle.com/technetwork/java/codeconventions-137265.html#587 There is one thing I don't really agree with. It is not only OK to make member variables public, it is a best practice, provided that you make them final. Instead of private String namel public String getName(){ return name; } it should be public final String name; public MyObject(String name){ this.name = name; } What this means is that instance variables are set in the constructor where ever possible. In general, we should favor immutable objects, as they are inherantly thread safe. It also means that we use the constructor to confirm that a given object complies with its contract. This is in Keeping with JOshua Block'ss advice in "Evffective Java" which should be required reading.
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
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
I had no idea that the naming of interfaces and member fields would generate such 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
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
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? 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
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
On 11/28/2011 09:27 PM, Nathan Kinder wrote:
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.
Which is precisely why encoding type information into a symbol name is a bad idea. -- John Dennis <jdennis@redhat.com> Looking to carve out IT costs? www.redhat.com/carveoutcosts/
On Tue, 2011-11-29 at 08:36 -0500, John Dennis wrote:
On 11/28/2011 09:27 PM, Nathan Kinder wrote:
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.
Which is precisely why encoding type information into a symbol name is a bad idea.
FWIW, it is possible to set the project formatting rules in eclipse to require member fields to be prefixed with "m" - and to replace all non-confirming instances automatically, as well as to flag inconsistencies going forward. So if we choose to continue to maintain this convention, we can be consistent. And renaming any particular variable is trivial in eclipse. I'm not arguing that this type of thing is a good idea in itself. But I also think that having code that contains member variables that have been prefixed as well as member variables that haven't been - is worse off - because it is inconsistent and no one will know whether to trust the "m" notation or not. We have a very large code base that by and large follows this convention. We can make it absolutely consistent by setting the eclipse formatting rules if we choose to continue to follow this convention. If we abandon the convention, we can do a few things: 1. We could try to replace all old instances all at once - which is a monumental and error-prone process. And one which quite frankly, we do not have the time to do. 2. We could replace variables in files as we work on them. 3. We could just leave the old variables and not use the convention in new code. Both 2 and 3 leave the code in an inconsistent and therefore, I would argue, less readable state. Ade
OK, here's why I think we should drop the 'm'. Hmmmm ....OK...let me back up.... An object should conform to a contract. Once the object is created, that contract has been signed, sealed and delivered. At least, that is the intention. However, Java often falls down on this. We have "Init" methods. We have Property setters that must be called before we have a "valid" object. Why? Java is a pretty strongly typed language. But, it needs to be able to interface with scripting languages, and thus it has, among other things, the reflection API. Reflection has the ability to enumerate the properties (member variables) of the object, and to fetch values from the object without having to have a compile time dependency. We can even make variables that are Read Only by tagging them as 'final'. A final field must be defined by the time the constructor has returned. The dependencies to decide the value for the final field must be passed in to the constructror. The constructor is available via reflection. the Parameters are avialable via reflection...but.... Not the names of the parameters. This is possibly the largest oversight in the Java language. Without named parameters, a scripting language cannot do anything to determine which of the values it has maps to which of the dependencies of the constructor. All it can do is try to pass them in the right order. So what happened? Java has a nasty covention called the bean API. A naming convention get<CapitalizedName> and set<CapitalizedName>. Want to make somethig read only, drop the set. Joshua Blcok sets out an important rule of thumb in Effective Java: Favor Immutable Objects. An immutable object is implicitly thread safe. The Bean API with its late initialzation of properties makes this impossible. However, we have hope. In recent versions of Java, the Attribute API makes it possible to automatically build classes that let us create a Dictionary to pass to the constructor based on the names of the parameters. There is also the possibility of getting named parameters into the Java standard in the future. Regardless, this is only really an issue for scripting languages. Our application is all Java. This means that we can use classes and type safety to strongly associate the dependecies of a constructor with the objects that can fulfil those dependencies. We don't need to play "Everything is a string" like Javascript does. We can have immutable objects, without the bean API. And we can expose the properties of those objects as final properties instead of get<CapitalizedName>. But this only makes sense if the names of the properties are the "good name". This has the potential to make code that is much more readable and concise, while increasing type safety. Yes, increasing type safety. If a string field is immutable, but it needs some business logic, say to confirm that it complies with a pattern, we create a class that encapsulated that format, and make an immutable instance of that class. The pattern is confirmed in the constructor. If we try to make an instance that does not conform, throw an exception. Here is how it is done in a beanAPI way: public class Certificate{ String fingerprint; String getFingerprint(){ return fingerprint; } String setFingerprint(String fingerprint) throws IllegalArgumentException { validateFingerprint(fingerprint); this.fingerprint = fingerprint; } } Note that the logic for fingerprint is encapsulated in the Certificate object, and cannot be cleanly separated out. Here's the refactored solution. public class Fingerprint{ private String internal; public Fingerprint(String) throws InvalidArgumentException{ ... } String toString(){ ... } } public class Certificate{ public final Fingerprint fingerprint; puiblic Certificate (Fingerprint fingerprint) { this.fingerprint = fingerprint; } }
On 11/28/2011 08: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?
Makes sense, although I think we can talk it out in this forum. Email, especially a publicly archived list like this, is actually a good way to drive he discussion, better, I've found, than a wiki. There are two issues here, though, as Christina points out: the automatable changes, which should be fairly non-contraversial, and the ones that will require manual changes. We are not going to automate the removal of the 'm' friom member variables and the like, so we can defer making a decision on those.
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
On 11/23/2011 02:57 PM, Ade Lee wrote:
instead of this (as in the Sun standard): public class MyClass { ... }
preferred, closer to K&R, you can more lines of code on the screen if you don't allocate one line just for a brace.
2. We require that interface names begin with "I". Sun says nothing about this. This is probably a good one to keep.
Using an initial character to declare a type was a bad idea over a half century ago when Fortran was invented, but of course there were justifiable limitations in the 1950's but those have vanished, oh let's say 30-40 years ago. Then some idiot at Microsoft came up with Hungarian notation, one of the worst ideas I've ever seen. In a strongly typed language there is absolutely no need to encode the type of the object in it's name, a throwback to a bad idea in Fortran which diminishes readability for no apparent useful reason. Let's not encode the type in the name. The name should be self evident and if you still can't figure out the type then look up it's declaration. I also don't believe in declaring interfaces unless there is a need. Excessive use of interfaces for which there is only one class implementing the interface just makes the code more verbose and harder to understand what's going on. The presence of an interface implies there is some public sharing between objects. Can't tell you the number of times I went looking for all the places an interface was used only to discover it was just one self contained class, go figure.
3. We specify "no tabs". Sun says tabs are optional. We should keep this.
No tabs. Everyone has a different idea of what tab width should be and if they aren't the same, which they never are, all you get is a jumbled mess.
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.
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".
All good.
* Member variables should begin with "m". For example, mMemberVariable.
Another incredibly annoying convention, see above rant about encoding type information in a symbol name. Objects essentially can only have member variables and methods, if someone can't figure out the difference then prefixing it with an 'm' is the least of their problems :-) -- John Dennis <jdennis@redhat.com> Looking to carve out IT costs? www.redhat.com/carveoutcosts/
Can't find Ade's oreigianl message, but regarding the m at the beginning of member variables: No. Just no. It is not needed, it makes things hard to change. If the scope is so difficult to track, there is something wrong with the class. No Systems Hungarian. No Applications Hungarian. None of it. It is not needed, does not help, and makes things hard to read. Besides, it sets everyone's teeth on edge. Let it go.
participants (7)
-
Adam Young -
Ade Lee -
Christina -
Endi Sukma Dewata -
John Dennis -
Matthew Harmsen -
Nathan Kinder