Coding conventions

[Edu Zamora]

Dear fellow developers,

I would like to define the coding conventions to use in AnkiDroid and
then create a formatter for Eclipse (and maybe a checkstyle) in
concordance with these conventions.

Coding conventions are important because they give consistency to the
code and when the reader is used to the style applied it allows him to
read, understand, debug and maintain the code faster that he would do.
But they are not so important to start any holy war, so I propose to
vote for the most important things, decide how we will do them and
move on:

- Indentation style (http://en.wikipedia.org/wiki/Indent_style)
- Indentation with spaces or tabs
- Use of whitespaces
- Anything else you would like to vote?

Once we define the most important things between all of us, I will
fill in the gaps, create the formatter and document everything the
best I can.

I would like to finish with a great sentence of Jeff Atwood about
coding conventions (from his article
http://www.codinghorror.com/blog/2009/04/death-to-the-space-infidels.html):
"Choose tabs, choose spaces, choose whatever layout conventions make
sense to you and your team. It doesn't actually matter which coding
styles you pick. What does matter is that you, and everyone else on
your team, sticks with those conventions and uses them consistently. "

Nothing else to add to Jeff :P

Cheers!

Edu Zamora

[Jan]

On Sep 20, 6:22 pm, Edu Zamora <edu.z...@gmail.com> wrote:
> Dear fellow developers,
>
> I would like to define the coding conventions to use in AnkiDroid and
> then create a formatter for Eclipse (and maybe a checkstyle) in
> concordance with these conventions.

to make things simpler, could i suggest google's own coding
guidelines?

http://source.android.com/source/code-style.html

i've just come across them recently, they are actually required if you
want to contribute any code back to android (google takes code
reviews / code style very seriously). we probably don't want to
contribute code to google but their recommendations make mostly sense.

in short (regarding style):

4 spaces, no tabs.
100 columns
Braces: Opening braces don't go on their own line.


jan

[Nicolas Raoul]

Adopting a convention is a good idea, just yesterday I was modifying
StudyOptions and hesitated on the style to adopt.

I usually prefer adopting Java's official guidelines, but as Jan said,
for an Android project it makes sense to adopt Android's guidelines,
especially since they seem reasonable.

However, I think we should not refuse contributions based on style.
Some people might be crazy coders who don't care about style, and
contribute excellent features. No problem, I can tidy up the code
while reviewing it, or anyone can do it.
I am a wikipedia addict, I could spend my life "wikifying" raw
articles created by newcomer geniuses, and enjoy it. Same for
AnkiDroid.

We can suggest the use of an Eclipse configuration file, but it must
not be another barrier for new developers.

Cheers!
Nicolas Raoul

> --
> You received this message because you are subscribed to the Google Groups "AnkiDroid" group.
> To post to this group, send an email to anki-a...@googlegroups.com.
> To unsubscribe from this group, send email to anki-android...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/anki-android?hl=en-GB.
>
>

[Marcus]

What is the rational for 100 columns? I understand everyone has
different resolutions, editor space, etc, but Anki uses 80 columns if
I remember right and all it does is clutter the vertical spacing of
the code.

For example, lets say a function and its arguments need to be split up
across three lines because of the artificial restriction. Now, the
99.9999% I am not interested in that section of code I have to scroll
through it and it clutters my mind with code that is unnecessary to be
seeing. And, the 0.0001% of the time I am interested in that code
scrolling vertically through it offers no greater advantage than
crolling horizontally through it.

Especially considering that horizontal position is how most cultures
determine relatedness with text. The word below this line may not
relate to this sentence, but you know that the word horizontally close
to this word most probably does relate. So, why create ambiguity with
an artificial restriction?

99% of the time code needs to split up across lines because of
function arguments. And 99% of the time you are looking at the code
you don't care what the 7th argument is and the 1% of the time you do
you know to hit the little scroll button and view it.

[Damien Elmes]

http://stackoverflow.com/questions/746853/the-80-column-limit-still-useful

[Marcus]

Most arguments on there compare the line length to printed books.
However, reading code and reading books are very different: Unlike a
book, code does not need to be read sequentially or be read
completely.

In addition, in a book, every position on a page has equal importance.
In code, the beginning of a statement is usually much more important
than what follows.

So, I think it is unwise to compare two things that have completely
properties. I guess it is a matter of preference and there seems to be
a sizeable number of people in both camps. I would vote for letting
the one who is writing the code use discretion in order to determine
what makes his code more or less readable.e

[Jan Berkel]

99% of the time code needs to split up across lines because of
function arguments. And 99% of the time you are looking at the code
you don't care what the 7th argument is and the 1% of the time you do
you know to hit the little scroll button and view it.

in my opinion writing very long lines is a code smell, so having a rule to enforce some limits makes sense. 

so regarding your example, you really should not write methods which take 7 or more arguments.

if you can, i recommend setting up your editor so very long lines get highlighted, so you're more aware of it.

  

  jan

  

[Edu Zamora]

> to make things simpler, could i suggest google's own coding
> guidelines?
>
> http://source.android.com/source/code-style.html

I think it is a great idea to take the Android code style rules like
base for our own, but I think we can modify some of them to suit our
needs better.

In general I like them but I would not be so strict about the max
column length, because I guess that no one of us is developing in
terminals or screens so small that need to set the max column length
to 80 characters to be able to read it easily.
With a limit of 120 characters per line (the max recommended by Robert
C. Martin in Clean Code) I can see all the code in my Macbook 13''
without having to scroll horizontally and in the computer I use at
work I can even see two files at the same time, side by side, without
having to scroll. More than enough.
(I agree that the example given by Marcus with 7 arguments maybe was
not the best one, but it's true that in Java, using descriptive names
for methods and variables, lines can get quite long).

So, I would rise the limit length for columns to 120 characters (and
if someday we had to comply with Android code styles for any reason,
it can easily done with a formatter).

> However, I think we should not refuse contributions based on style.

Of course not, if someone is a casual contributor maybe he does not
even know about the coding conventions we use, and the members of the
team can format the code later. No problem with that. In other hand,
all of us, like usual contributors, should stick to them, so we can
benefit from its advantages.

Anyone is working with an IDE different for Eclipse? In that case, we
should try to create a formatter for that IDE also.

[iniju]

> Anyone is working with an IDE different for Eclipse? In that case, we
> should try to create a formatter for that IDE also.

Not really an IDE, but Vim. I'll see what I can do about a formatter.
By the way, I have no preference on style, but I strongly support
having some coding convention.

K

[Diogo V. kersting]

On Tue, Sep 21, 2010 at 10:29 AM, iniju <inigo....@gmail.com> wrote:

Not really an IDE, but Vim. I'll see what I can do about a formatter.
By the way, I have no preference on style, but I strongly support
having some coding convention.

 

I also use vim, except when programming in java... then I use eclipse.

--
----------------------
Diogo V. Kersting - Epidemus LTDA
Desenvolvedor do BRLix

[iniju]

How about logging?
I believe we remove some logging levels from production releases,
which ones?

I also suggest removing logging inside loops once the debugging
testing phase is done, as they reduce performance and clutter.

Also, is there a reason for having more than one logging tags? Most of
our classes use tag AnkiDroid, but not all.

Cheers
Kostas


On Sep 21, 9:37 am, "Diogo V. kersting" <k...@brlix.com> wrote:

[Edu Zamora]

> How about logging?

Very interesting subject, Kostas.

We removed all logs for some releases but I think in some others we
did not remove any log. We didn't standardize that, so it was a little
haphazard.

I think we have two options regarding removing logs:
1) Remove all logs, in order to maximize performance
2) Remove all logs but the ones that could help us pinpoint possible
bugs or help users when they have problems

I prefer option two, although it will imply a manual process to see if
the logs are relevant or not. What do you prefer? Do you see a better
solution?

> I also suggest removing logging inside loops once the debugging
> testing phase is done, as they reduce performance and clutter.

Totally agreed.

> Also, is there a reason for having more than one logging tags? Most of
> our classes use tag AnkiDroid, but not all.

The reason to use more than one logging tag would be that then you can
filter by tag on DDMS, but I don't know if anybody needs or uses that.
Personally, at most I filter to only see AnkiDroid logs, so for me
using only AnkiDroid tag is enough (I thought we only had this tag.
Which other tags do we have?)

[iniju]

I also support option 2, some logs are fine. It's up to the developer
to decide, as he/she knows best which parts of the code are important
and which ones might need some debugging in the future.

Regarding the other log tags:
StudyOptions has Ankidroid (probably a typo, very misleading)
SharedDeck has AnkidroidSharedDecks
ErrorReporter has ErrorReporter
CustomExceptionHandler has CustomExceptionHandler
Connection has Connection
and all the Veecheck ones use veecheck

veecheck uses this way to enforce the same tag to all its classes
without having to redefine it:
import static com.tomgibara.android.veecheck.Veecheck.LOG_TAG;

Kostas

[Martin André]

Thanks Edu for bringing up the subject. Would it make sense to also
have naming rules for files? I am thinking about classes extending
activities for example.

Cheers,
Martin

[Edu Zamora]

> Would it make sense to also
> have naming rules for files?

It could make sense. Do you have something in mind like a suggestion?

[Martin André]

On Sep 22, 6:57 pm, Edu Zamora <edu.z...@gmail.com> wrote:
> > Would it make sense to also
> > have naming rules for files?
>
> It could make sense. Do you have something in mind like a suggestion?

Nothing fancy. Just adding an Activity suffix to the activity related
classes:
- About.java would become AboutActivity.java
- CardEditor.java would become CardEditorActivity.java
- and so on...

Martin

[Damien Elmes]

Long lines make it hard to have multiple files or diffs open on one
screen. I frequently work with more than one file at once (something
from ankiqt and libanki at the same time). Aside from that, it's also
in the official Python coding conventions.

[Marcus]

I agree. The biggest offender when I look at anki code is sql
statements. I still am not used to seeing sql statements broken up
across many lines. This is why I say discretion is good. 7 arguments
is probably an indicator of a need to refactor, but an sql statement
no so much. IMO at least.

[Marcus]

>>(I agree that the example given by Marcus with 7 arguments maybe was
>>not the best one, but it's true that in Java, using descriptive names
>>for methods and variables, lines can get quite long

That is one of the biggest reasons why I dislike enforced column
limits. Once you start adding in indentation, and method names, etc
you only have so much room, so developers start to shorten the names
of symbols. A policy meant to increase readability now just made the
code less descriptive.

[Edu Zamora]

Hi everyone,

I have spent the last days writing down the Code Conventions for
AnkiDroid and they are already available at:
http://ankidroid.googlecode.com/files/AnkiDroid-Code.Conventions.zip

These conventions are not set in stone for now, so if you see
something that you don't like, that you think it does not make sense
or something that you would like to add feel free to say it here.
Also, if you see any kind of error it would be great if you can report
it to me.

Additionally, I have created a Checkstyle configuration file for
Eclipse (you will need to install the Checkstyle plugin in order to
use it) and configuration files for Formatter, Clean Up and Organize
Imports (if you execute "Source" > "Clean Up..." in Eclipse, the code
will be automatically formatted to comply most of the guidelines).
Again, if you see any inconsistency between the Code Conventions, the
Checkstyle and the files used to format the code or you know how to
improve some of them, don't hesitate to let us know. These
configuration files can be found here:
Checkstyle configuration file: http://ankidroid.googlecode.com/files/ankidroid.checkstyle.xml
Formatter + Clean Up + Organize import configuration files:
http://ankidroid.googlecode.com/files/AnkiDroid%20Formatter%2C%20Clean%20Up%20and%20Import%20Order.zip

And now, I will spend the rest of the morning formatting the whole
project to comply as best as I can with the conventions, so we won't
have so much work in next commits.


Cheers!

Edu Zamora

[iniju]

Hi Edu,

Great work! Still reading through it and I stumbled on an ambiguous
pair of recommendations:
40. One blank line should be used in the following circumstances:
- Between the local variables in a method and its first statement.
- Before a block or single-line comment.
- Between logical sections inside a method, to improve readability

85. Variables should be initialized where they are declared and they
should be declared in the smallest scope possible.

The first one of these implies that the local variables in a method
should be declared at the beginning of the method (followed by a blank
line for readability).
The second one advises that variables should be declared in the
smallest scope possible, which is most of the times not the beginning
of the method, but instead some inner scope of if or loop.

I'm not sure what kind of optimizations Java does, but I would advise
that the rule for smallest scope possible has an exception: It's
better to declare variables that are used inside a loop (if possible)
just before we enter the loop, so we avoid possible redundant
constructor/destructor calls.

Great stuff, thanks :)
Kostas

On Sep 29, 6:08 am, Edu Zamora <edu.z...@gmail.com> wrote:
> Hi everyone,
>
> I have spent the last days writing down the Code Conventions for
> AnkiDroid and they are already available at:http://ankidroid.googlecode.com/files/AnkiDroid-Code.Conventions.zip
>
> These conventions are not set in stone for now, so if you see
> something that you don't like, that you think it does not make sense
> or something that you would like to add feel free to say it here.
> Also, if you see any kind of error it would be great if you can report
> it to me.
>
> Additionally, I have created a Checkstyle configuration file for
> Eclipse (you will need to install the Checkstyle plugin in order to
> use it) and configuration files for Formatter, Clean Up and Organize
> Imports (if you execute "Source" > "Clean Up..." in Eclipse, the code
> will be automatically formatted to comply most of the guidelines).
> Again, if you see any inconsistency between the Code Conventions, the
> Checkstyle and the files used to format the code or you know how to
> improve some of them, don't hesitate to let us know. These
> configuration files can be found here:
> Checkstyle configuration file:http://ankidroid.googlecode.com/files/ankidroid.checkstyle.xml

> Formatter + Clean Up + Organize import configuration files:http://ankidroid.googlecode.com/files/AnkiDroid%20Formatter%2C%20Clea...

[Edu Zamora]

Hi Kostas,

> The first one of these implies that the local variables in a method
> should be declared at the beginning of the method (followed by a blank
> line for readability).

Although that is not what I meant when I wrote it, I see now that it
can seem that implies that. What I wanted to convey, is that if some
variables are declared at the beginning of the method these variables
should be separated from the first statement with a blank line. Not
that all variables should be declared at the beginning of the method.

If that were the case, guideline #98 would also be inconsistent:

98. Loop variables should be initialized immediately before the loop.

> The second one advises that variables should be declared in the
> smallest scope possible, which is most of the times not the beginning
> of the method, but instead some inner scope of if or loop.

Here scope can be misleading. Maybe it would be better to say that
variables should be declared as close as where they are used, if it
makes sense. How you pointed really well (and how guideline #98 says)
if the variable does not depend on the iteration of the loop, it is
always better to declare it just before it (it would not make sense
declare it inside it and repeat the same operation over and over).

So, how you would rewrite the guidelines you mention to make them more
clear and avoid ambiguity for the reader?

Thank you so much for your feedback Kostas!

[Edu Zamora]

Also, while I was applying the conventions yesterday I saw some things
that maybe we could include, like:

- In AsyncTasks, put the methods onPreExecute, onProgressUpdate and
onPostExecute in that order (the natural order they are executed)
- Not only group the lifecycles methods but also all Android methods.

And regarding the formatter, I was thinking in deactivate the
automatic wrapping of lines because I didn't like how some lines were
broken and I think it is a call that is better leave to the
programmer.

What do you guys think?

[Nicolas Raoul]

> - In AsyncTasks, put the methods onPreExecute, onProgressUpdate and
> onPostExecute in that order (the natural order they are executed)
> - Not only group the lifecycles methods but also all Android methods.

Yes, that would be nice!

> And regarding the formatter, I was thinking in deactivate the
> automatic wrapping of lines because I didn't like how some lines were
> broken and I think it is a call that is better leave to the
> programmer.

Indeed, I don't really like automatic line wrapping, for instance if
there are 4 similar lines, and one of them is 81 characters wide, it
makes more sense to not wrap it, but an automatic formatter would not
understand the aesthetic here.

Great work overall!
I guess you will put it somewhere online more visible than within a
ZIP archive? As a web page? Maybe as a wiki page, so that the
community can maintain it? (but that would require transforming the
formatting).

Once again, newcomers should not be afraid by the convention.
You can ignore all rules, contribute code that does not respect any
convention, and we will still be very thankful :-)

Hopefully, some Good Samaritan will come along from time to time and
reformat the unconventional pieces.
Actually, reformatting the code is quite easy and does not require a
deep understanding of the code.
Anyone willing to reformat the current code according to the convention?
That's a good way for a newcomer to have a first experience with
AnkiDroid's code :-)
Any volunteer?

Thanks a lot!
Nicolas

[Nicolas Raoul]

Here is how to use the Formatter + Clean Up + Organize import
configuration files:

Download http://ankidroid.googlecode.com/files/AnkiDroid%20Formatter%2C%20Clean%20Up%20and%20Import%20Order.zip
Unzip it.

In Eclipse, Window→Preferences
Section Java→Code Style→Clean Up
Import, select ankidroid.cleanup.xml

Section Java→Code Style→Formatter
Import, select ankidroid.formatter.xml

Section Java→Organize Imports→Formatter
Import, select ankidroid.formatter.xml
Press OK.

Nicolas

[iniju]

> - In AsyncTasks, put the methods onPreExecute, onProgressUpdate and
> onPostExecute in that order (the natural order they are executed)
> - Not only group the lifecycles methods but also all Android methods.

That reminds me, maybe we can add a guideline to use annotation
@Override when we override a method. It improves readability and can
safeguard from really hard to find errors.

[iniju]

For those using Vim, the following file does indentation work more in
line with the coding conventions:
http://ankidroid.googlecode.com/files/java.vim

It's basically just the default java intent file from vim 7.2, with
two additions:
- Space-based indentation for java files, 4 spaces.
- Proper indentation for annotations @... that is no indentation after
annotations eg:
@Override
onPreExecute(...

Still doesn't do properly the indentation of split lines with unclosed
parentheses, but it should work in most of the cases.

The file should be placed in ~/.vim/indent/java.vim and the following
line must be included in the .vimrc file:
filetype plugin indent on

Cheers
Kostas

[iniju]

I need a clarification on #37:
37. White space should be used in the following cases:
- After and before operators.

Does that include the negation operator ! (no objection, it's just
that I don't see it often like that).
if (! foo.isValid()) {
...
}

or

if (!foo.isValid()) {
...
}

Kostas