dbgroup-at-ucsc / dbtune

This research project aims to develop tools in order to make index tuning easier and more effective.
http://users.soe.ucsc.edu/~alkis/tuning/
Other
5 stars 3 forks source link

Pick a coding convention for the project #51

Closed ivotron closed 12 years ago

ivotron commented 13 years ago

We should have an explicit coding convention for our project.

ivotron commented 13 years ago

I like this one a lot. They even provide a rule-file for using it with checkstyle

The only thing I'd change is the javadoc style from this:

/**
 * Creates a column with the given name and type. The type should
 * be one of the values defined in SQLTypes.
 *
 * @param name name assigned to the column.
 * @param type data type
 * @return returns a reference to the object
 * @throws InvalidArgumentException when the argument passed to it is wrong
 */

to this:

/**
 * Creates a column with the given name and type. The type should
 * be one of the values defined in SQLTypes.
 *
 * @param name
 *     name assigned to the column.
 * @param type
 *     data type
 * @return
 *     returns a reference to the object
 * @throws InvalidArgumentException
 *     when the argument passed to it is wrong
 */

the second one is more readable.

Also, the blocking code convention I'd suggest for us to use is:

if (i == 0)
{
    assertEquals("movieid", columns.get(i).getName());
}
else if (i == 1)
{
    assertEquals("title", columns.get(i).getName());
}

that is, a brace in its own line.

Also, I'd like to suggest to avoid by all means using one-liners. I've noticed that code in the core has many of these one-line statements. For example, in DB2IndexMetadata Jeff and I were having a hard time trying to understand the following:

return whatIfOptimizer.whatIfOptimize(sql.toString())
                    .using(new DefaultBitSet(), new DefaultBitSet())
                    .toGetCost();

The guidelines that the convention specifies for conditionals (guideline 53) applies to the above:

"Complex conditional expressions must be avoided. Introduce temporary boolean variables instead"

Lastly, I think all methods, regardless of their simplicity, should be documented. This applies to methods included in unit tests, in which case the comment should provide a high-level description of what the test is checking, eg.:

/**
 * Checks that the ordering of columns on each table and index is
 * correct. The test makes sure that the elements on the list that
 * is returned by a container (eg.  columns), when
 * iterated using the List.iterator() method, are in
 * the correct order.
 */
@Test
public void testColumnOrdering() throws Exception
{
    .
    .
    .

I think that actually following a coding convention is more important than picking one. Picking one is easy, following one and keeping the code in synch with it is the difficult part.

ivotron commented 13 years ago

I forgot. 4 space indentation XD

hsanchez commented 13 years ago

We should follow a coding standard that is similar to the one used at Sun (now Oracle). Here is the one recommended by Doug Lea (the creator of the concurrency api in Java)

http://gee.cs.oswego.edu/dl/html/javaCodingStd.html

hsanchez commented 13 years ago

Ah forgot, I would suggest moving away from C#'s recommended coding convention, e.g. if(condition) {

....

}

or public void nameOfMethod(..) {

....

}

Honestly, I don't like that convention. But this may be a matter of taste. This is why I am recommending a standard that was/is used by Sun: the one by Doug Lea.

ivotron commented 13 years ago

My goal for opening this issue wasn't to initiate a big discussion on this. I wanted us to agree quickly on a convention so that we can start following it ASAP. The convention I posted states it nicely (section 1.1):

The motivation section is important. Coding standards and guidelines tend to start "religious wars", and it is important to state the background for the recommendation.

Having said that. I'd like to know the "why's". Why should we use a convention from Sun? The link you posted is more of a set of high-level recommendations rather than a style convention. I'd prefer to use one that is well-documented (with sample code) and easy to follow. I'm not saying we should use the one I posted, I'm just saying we should pick something that is at least as well-documented as that one from geosoft.

I agree that the blocking style is a matter of taste, in my opinion the C# style is more readable (I wrote C/C++ a lot during my undergrad studies).

hsanchez commented 13 years ago

It is the reason "they tend to star religious wars" that I am saying that it is important to follow some basic recommendations rather than a set of guidelines. For example, comment your code, don't leave broken Javadocs, use camel casing when naming classes/methods/etc., and use indentation... etc

And regarding the "why's" we should use Doug Lea's. Simply because they are just merely recommendations and not a set of style conventions, which developers tend not to follow because they either might have skimmed or have not read the entire document of conventions.

Honestly, as you said before that we are falling behind schedule, I would rather focus on finishing our work than reading an entire doc and making sure that I am following each bullet point in my code, which is not productive.

ivotron commented 13 years ago

We don't have to waste time on this. Checkstyle provides an ant task, so we can make it part of our development env.

Having a set of high-level guidelines is almost as if we hadn't any convention at all. The goal of having one is to "homogenize" code to enhance readability. With high-level guidelines code written by a member of the team will look entirely different to the one written by others.

hsanchez commented 13 years ago

I disagree with your assessment. These recommendations help developers to focus on writing code, which is what it matters, rather than making sure that their code matches some "bullet point by bullet point" coding convention.

Additionally, how would you addressed the fact, as it is a matter of taste of course, that certain developers (including myself) believe that C#, C/C++ coding convention makes Java code unreadable? It would be hard to address that since you disagree with me. That is why we should find some balance. And this balance could be found in these sort of recommendations.

I will be happy to discuss my position with you offline. Please let me know.

ivotron commented 13 years ago

If you configure your editor correctly you don't need to worry about this.

(editor) + (ant task) = readable code for free

I don't mind using Java-style code blocks. Any other thing you disagree with the convention I posted? I'm optimistic and think that finding a balance is easy.

hsanchez commented 13 years ago

Hey, Ivo,

The following link points to the Java coding conventions that Sun (now Oracle) recommends others to follow when writing Java code. We could use the best things that the coding convention you suggested offers and some of the Sun's ones. What do you think?

http://www.oracle.com/technetwork/java/codeconv-138413.html

How hard would it be to tweak checkstyle to follow a mix of both conventions?

ivotron commented 13 years ago

I like Sun's style also. The only two differences I can find between the two is the tab length (2 in geosoft's; 4 in Sun's) and one-line if,for,while statements is prohibited by Sun's. They seem to agree on everything else, it's just that geosoft's has more details about it.

Things we agree on:

The only thing we'd need to agree on is the line length. I don't have a preference on this, although I'd like to suggest anything that doesn't go over 100 chars.

Regarding customizing the checkstyle configuration file, it's straight forward.

ivotron commented 12 years ago

Sample Checkstyle output:

ivo@reynalda:~/projects/dbtune
$ ant test
Buildfile: build.xml

clean:

checkstyle:
[checkstyle] Running Checkstyle 5.5 on 76 files
[checkstyle] src/edu/ucsc/dbtune/workload/Workload.java:1: Missing BSD License
[checkstyle] src/edu/ucsc/dbtune/workload/Workload.java:19: Wrong order for 'java.io.BufferedReader' import.
[checkstyle] src/edu/ucsc/dbtune/workload/Workload.java:21:1: Redundant import from the java.lang package - java.lang.Iterable.
[checkstyle] src/edu/ucsc/dbtune/workload/Workload.java:34:5: Missing a Javadoc comment.
[checkstyle] src/edu/ucsc/dbtune/workload/Workload.java:49:63: ',' is not followed by whitespace.
[checkstyle] src/edu/ucsc/dbtune/workload/Workload.java:49:76: '{' should be on a new line.
[checkstyle] src/edu/ucsc/dbtune/workload/Workload.java:57:14: 'while' is not followed by whitespace.
[checkstyle] src/edu/ucsc/dbtune/workload/Workload.java:62:15: 'if' is not followed by whitespace.
[checkstyle] src/edu/ucsc/dbtune/workload/Workload.java:66:15: 'if' is not followed by whitespace.
[checkstyle] src/edu/ucsc/dbtune/workload/Workload.java:67:74: '-' is not preceded with whitespace.
[checkstyle] src/edu/ucsc/dbtune/workload/Workload.java:67:75: '-' is not followed by whitespace.
[checkstyle] src/edu/ucsc/dbtune/workload/Workload.java:80: Expected an @return tag.
[checkstyle] src/edu/ucsc/dbtune/workload/Workload.java:80:36: '{' should be on a new line.
[checkstyle] src/edu/ucsc/dbtune/workload/Workload.java:88:46: '{' should be on a new line.
[checkstyle] src/edu/ucsc/dbtune/workload/package-info.java:17: Line is longer than 100 characters.
[checkstyle] src/edu/ucsc/dbtune/workload/package-info.java:18: Line is longer than 100 characters.
npolyzotis commented 12 years ago

Can checkstyle fix these "errors" automatically?

ivotron commented 12 years ago

AFAIK, it doesn't. I look for this and it seems that the reason of not having automatic resolution of "errors" is that the design assumption behind Checkstyle is that IDEs or text editors can take care of this automatically.

npolyzotis commented 12 years ago

Then maybe we can specify how to enforce the style through our editors? For instance, do we know how to enforce this style through eclipse?

On Dec 6, 2011, at 4:54 PM, Ivo Jimenez wrote:

AFAIK, it doesn't. I look for this and it seems that the reason of not having automatic resolution of "errors" is that the design assumption behind Checkstyle is that IDEs or text editors can take care of this automatically.


Reply to this email directly or view it on GitHub: https://github.com/dbgroup-at-ucsc/dbtune/issues/51#issuecomment-3041581

ivotron commented 12 years ago

There are many plug-ins for IDEs (Eclipse, IDEA, NetBeans, etc) that take a Checkstyle configuration file and mark the style errors in the editor of the IDE. For the particular case of Eclipse:

  1. How to install the plugin
  2. Configure it

In step 2, we'd point the configuration to our checkstyle.xml file that contains the definition of our convention.

npolyzotis commented 12 years ago

I am fine with enforcing a particular style. Anybody else who objects? (Do these messages go out to everybody?)

On Dec 7, 2011, at 1:36 AM, Ivo Jimenez wrote:

There are many plug-ins for IDEs (Eclipse, IDEA, NetBeans, etc) that take a Checkstyle configuration file and mark the style errors in the editor of the IDE. For the particular case of Eclipse:

  1. How to install the plugin
  2. Configure it

In step 2, we'd point the configuration to our checkstyle.xml file that contains the definition of our convention.


Reply to this email directly or view it on GitHub: https://github.com/dbgroup-at-ucsc/dbtune/issues/51#issuecomment-3045090

ivotron commented 12 years ago

They are seen in the Dashboard and to those subscribed to this issue (only Huascar, you and me). I asked Trung and he doesn't have any preferences over a particular style. Neither do I, but IMO I think we should use the same across the project. This style already provides a checkstyle configuration file (which I used to generate the output I posted).

npolyzotis commented 12 years ago

Fine with me -- let's go with the suggested style.

On Dec 8, 2011, at 1:11 AM, Ivo Jimenez wrote:

They are seen in the Dashboard and to those subscribed to this issue (only Huascar, you and me). I asked Trung and he doesn't have any preferences over a particular style. Neither do I, but IMO I think we should use the same across the project. This style already provides a checkstyle configuration file (which I used to generate the output I posted).


Reply to this email directly or view it on GitHub: https://github.com/dbgroup-at-ucsc/dbtune/issues/51#issuecomment-3059555