Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coding standards #554

Open
Lekensteyn opened this issue Aug 2, 2011 · 28 comments
Open

Coding standards #554

Lekensteyn opened this issue Aug 2, 2011 · 28 comments

Comments

@Lekensteyn
Copy link
Contributor

Let's establish some coding standards as the current code base is getting horrific.

The current proposal is visible at:
https://github.com/MrMEEE/bumblebee/wiki/Coding-Standards

Please discuss before changing.

@Samsagax
Copy link

Samsagax commented Aug 2, 2011

Totally agree with this. It's a good way to make the code more readable. I'll also suggest some blank lines and a starting comment on what are we trying to acomplish withing a block:

Something like this:

# I'm saying hello
some code to raise my hand.
wave it
if wavedback; then
    say hello
fi

Also, for the record, I prefer 3 spaces instead of 4 for indentation, but is just a number in the tab substitution.

@ArchangeGabriel
Copy link
Contributor

Also totally agree with Lekensteyn. Including 4 spaces for indentation.

And please when every one will be ok, let me correct actual code this way, I have a great idea on how to make it very fast. And as I am maniac, I will really enjoy it :P

@Lekensteyn
Copy link
Contributor Author

sed? :p

Wiki created: https://github.com/MrMEEE/bumblebee/wiki/Coding-Standards
Does everyone agree with it? About variable definition, should we allow:

local a=1 b=2

or must it be:

local a=1
local b=2

or even:

local a b
a=1
b=2

Another thing, I've always tried to keep the lines within 80 characters unless it became unreadable or such. Your opinions?

@ArchangeGabriel
Copy link
Contributor

A friend of mine also do that. Originally, this is for printing purposes using vim.

But we can use it as a standard for readability.

For local, both first and second are convenient for me. So I let the others choose.

And no, not sed, as I'm newby to the Linux world and don't now what are the "basics". I would to that using N++ under windows.

@Samsagax
Copy link

Samsagax commented Aug 2, 2011

For variables I'd prefer the second or third options for readability. For line lengths I always put one line per statement except in really short 'if' statements:

if condition; then; function_call; fi

That will always keep the lines under 80 chars unless some very large expression. In such cases a temporary variable would become in handy to break the function in smaller pieces.

@ArchangeGabriel
Copy link
Contributor

So let's use the second for variables.

@Lekensteyn
Copy link
Contributor Author

Included the 80 char limit, local a=x b=y deprecation and if condition; then something; fi.
That could be shortened to condition && something. I prefer using that, is it forbidden to use such constructs, or am I allowed to?

@ArchangeGabriel
Copy link
Contributor

Yes you can, I will do the same.

@Ximi1970
Copy link

Ximi1970 commented Aug 3, 2011

Just to be clear. Are you going to use a tab ( 4 spaces wide ) or are you going to use 4 spaces as indent? I hope you are not even thinking about the last option.....It really s*cks when trying to indent a selected block of text with [shift]--tab.

And it is a great idea to implement a standard. In our company we implemented it about 20 years ago. It is much easier for every one to read / understand the code and add some new stuff. One of our best rules: Keep the code spacy, use A LOT of spaces in your lines and please try to add some comments.
So do not generate oneliners like this:

main(int c,char*_v){return!m(v[1],v[2]);}m(char_s,char_t){return_t-42?_s?63==_t|_s==_t&&m(s+1,t+1):!_t:m(s,t+1)||_s&&m(s+1,t);}

( for the sed command this is almost impossible to live by, sorry about that :-) )

@ArchangeGabriel
Copy link
Contributor

In fact we were going to use 4 spaces, not a tab.

Because you say that a tab is 4 spaces, but that is almost false, the tab size or even encoding is system-related and even sometimes editor-related.

Not the space.

@Ximi1970
Copy link

Ximi1970 commented Aug 3, 2011

4 spaces was just an example for the size. Everybody can choose his own indent size when using tabs. Mine is 4 spaces wide. Somebody else can use 2 space wide tabs. It really does not matter what the other guy/girl is using as ident width.
You will edit the file always with your OWN indentation. It is therefore unnecessary to define an ident size when tabs are used.

@ArchangeGabriel
Copy link
Contributor

How do you set the tab size ?

And for information, when I edit something under Windows and use tab for indentation, they are replaced by 2 tabs under Linux...

@Ximi1970
Copy link

Ximi1970 commented Aug 3, 2011

In Windows:
Visual Studio you can find it here: Tools->Options->Text Editor->All Languages->Tabs or Tools->Options->Text Editor->< the language you are using->Tabs
UltraEdit: Advanced->Configuration->tab Edit

Linux:
Kwrite - >Settings->Configure Editor->Editing->Tab Identation
Qt creator->Tools->Options->Text Editor->Behavior

@ArchangeGabriel
Copy link
Contributor

Ok !!

I wasn't using one of this programs but I found the equivalent. I've just realized I had never look at gedit options...

But as Lekensteyn said, there is an option that should help you :
Indentation cluster: four spaces (not tabs, you can change the tab behavior on most editors to produce spaces when hitting TAB key)

And I confirm it works with MAJ-Tab.

@Ximi1970
Copy link

Ximi1970 commented Aug 3, 2011

Yes I know. They are going to all lengths to use spaces as tabs. Just WTF....

@Lekensteyn
Copy link
Contributor Author

Most modern editors allow for transparent removal of indentation, so that pressing backspace takes four spaces indention away. Have your considered the other points in the standard? It's not limited to indention.

I've used tabs a while a go, but switched to spaces to get consistent display in different programs (editors, browser, printing)

@Ximi1970
Copy link

Ximi1970 commented Aug 3, 2011

I normally (for C,C++) use this syntax:

if( x > 0 ) then
{
    printf( "Hello" );
}

We do not allow people to use the short forms like this

if( x > 0 ) then printf( "Hello" );

because it can lead to bugs when people are adding code too fast and try this:

if( x > 0 ) then printf( "Hello" );
    printf( " world!" );

instead of

if( x > 0 ) then
{
    printf( "Hello" );
    printf( " world!" );
}

Putting the { and } both on the indent line will also make it more readable and define the block better.

And I really like spaces :-) so this:

for ((i=0; i<$length; i++)); do

should be:

for (( i = 0 ; i < $length ; i++ )) ; do

@ArchangeGabriel
Copy link
Contributor

And does not render spaces.

But I can view the raw data of your post, and I agree with everything you've said in it. Including "I really like spaces".

If you really like them, why don't you use them as tabulation ? :P

@Ximi1970
Copy link

Ximi1970 commented Aug 3, 2011

Because the "old" editors I used did not have the space replace options......And if you want to indent files with that kind of editor, please be my guest. Especially when the files are very large...

@Ximi1970
Copy link

Ximi1970 commented Aug 3, 2011

Another syntax I am not going to recommend: putting the ';' on position 80 to mark the end of the lineprinters reach. Code beyond the 80 would not be printed and could not be seen when using the MSDOS debugger. :-)

@Lekensteyn
Copy link
Contributor Author

@ximi: github uses Markdown which was designed to be easy. Use four leading spaces to start a code block or use three backticks like:

````
code
```

I don't mind using tabs instead of spaces, as long as everyone sticks to it and do not mix those up.

My editor shows a line in the background for the X characters limit ;) (with X defaults to 80)

@ArchangeGabriel
Copy link
Contributor

@Ximi1970 : we're not putting a ; on position 80, just going to the next line if able.

@Ximi1970
Copy link

Ximi1970 commented Aug 3, 2011

Just joking :-)

@Ximi1970
Copy link

Ximi1970 commented Aug 3, 2011

@Lekensteyn: Thanks for the info, case of RTFM Ximi1970. Never have time for reading manuals :-)

Yes that is the most important part: do not mix styles.
Problem is: all my editors are tab "minded" for my work. I have to be carefull when I am going to work on bumblebee.

@Samsagax
Copy link

Samsagax commented Aug 4, 2011

Should be a good idea to say something about Commit messages (as they are part of coding):

Should be a line with the idea involving the present change, a blank line and then some more in-depth explanation on the change (not a bible, but some hint to others) so we don't have to diff every file. Unless, of course the change is little and obvious.

A really bad commit message:

Deleted some stuff

A good one:

Deleted some power-management useless files

  - Files under pm/useless are all gone
  - Files under pm/maybeused are moved to pm/used

@Lekensteyn
Copy link
Contributor Author

I second that, the commit message should be clear and useful. Consider http://stackoverflow.com/q/43598/427545

Keep the commit message within 72 characters, split over lines if necessary.

Fixed data loss bug

(optional additional description)

- Fixed a bug which could lead to data loss if it's night and the user 
  about to commit some changes (Closes: GH-123)

- Removed obsolete references to Prime-NG (GH-554: Clean Up)

Note the use of GH-<number>, it links to an issue. If it's Closes: GH-<number>, it also closes issue <number>. An extra newline should exist between every new line.

The PPA changelog needs some work too, not just "based on changes XXX", but:

PACKAGE (VERSION) UNRELEASED; urgency=low

  * New upstream release.
    - Message based on git changelog, that's why we expand it right?
    - Fix a bug which could lead to data loss (GH-123)
  * Main point (mind the two spaces in front of it)
    - some clarification (mind the four spaces in front of it)
    - we do not need to include all changes here, especially changes to whitespace

 -- User Name <[email protected]>  Thu, 04 Aug 2011 10:34:35 +0200

@ArchangeGabriel
Copy link
Contributor

Ok, will try to do that from now.

@Samsagax
Copy link

Samsagax commented Aug 5, 2011

Some further notice about coding:
We must use the 'install' command instead of 'cp' + 'chmod'. Some distros even advice that usage for packaging. So we can set permissions and create folders only if needed:
example: install bumblebee-config in /usr/bin owned by root with rw permission, read for the rest and execute by all:

install -D -m755 bumblebee-config /usr/bin/bumblebee-config

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants