Skip to content
This repository has been archived by the owner on Feb 21, 2021. It is now read-only.

Discussions about Programming Style and Coding Conventions #22

Closed
hustcalm opened this issue Mar 25, 2015 · 5 comments
Closed

Discussions about Programming Style and Coding Conventions #22

hustcalm opened this issue Mar 25, 2015 · 5 comments

Comments

@hustcalm
Copy link
Collaborator

I'm reading the source code of JdeRobot and wondering if we got a common programming style guide or coding conventions? I do find here is a Programming Style section, but not talking too much except for Doxygen.

For better collaboration, the coherent of coding conventions are important, especially for new developers trying to help and contribute source code.

I find two good references and hopefully we can have some discussions, then come out with our own JdeRobot Programming Style and Coding Conventions according to the existed code base. Maybe also do some code refactoring.

The references are Google C++ Style Guide and C++ Programming Style Guidelines. They mainly cover C++, we can also make adaptions to Python and Java, etc.

Cheers,
Lihang

@abhinavjain241
Copy link

True, I think @hustcalm pointed it out well. We should follow the Google C++ Style Guide for contributing to the project. This would help new developers too.

@JdeRobot JdeRobot added the bug label Mar 25, 2015
@rocapal rocapal assigned rocapal and unassigned rocapal Mar 25, 2015
@rocapal
Copy link
Member

rocapal commented Mar 25, 2015

That's true. We need to decide the same style guide for everyone. Google C++ Style Guide could be nice.

I have found this script [1] in order to check a C++ file and the steps to integrate it in eclipse [2]. And you know what? We have a lot of work with this issue :-)

So, we can wait a few days for other opinions and decide the final style guide before 1 week.

Thanks for your comments guys!

[1] https://google-styleguide.googlecode.com/svn/trunk/cpplint/cpplint.py
[2] http://choorucode.com/2015/01/08/how-to-use-cpplint-in-eclipse-cdt/

@hustcalm
Copy link
Collaborator Author

Cool, this can be a huge project refactoring the source code to be consistent with the Style Guide.

Quite agree @rocapal that we need more opinions and discussions to make things clear. However, before refactoring the existed code base, I suggest that we should first follow the style guide when contributing new source code once the final style guide decided.

For command line usage with CppLint, try [1]. However instead using it in Eclipse, I'm thinking about setting up git pre-commit hooks or something to run CppLint whenever a commit heppens. I'm already on it and hopefully will come out with a solution.

Also, it inspires wondering maybe Jenkins should help us too, see the issue I created for discussions [2].

[1] http://www.sw-engineering-candies.com/snippets/cpp/hello-world-google-cpp-style-guide-compliant
[2] #28

@rocapal rocapal removed the bug label Mar 26, 2015
@hustcalm
Copy link
Collaborator Author

@rocapal
I just had a survey about using git pre-commit hook tolint the source code to make sure that proper programming style and coding conventions are applied. After searching for a while, several alternatives came to my mind. Every open source project tends to have their own coding conventions, like [1], they also follow Google C++ Style Guide and using astyle[2] to ensure consistent coding style. Their pre-commit script can be found at [3], and looks like:

#!/bin/sh
#
# This hook script runs the astyle program in order to apply the style defined by ../../utils/astylerc_ansi
#
#
# the astyle configuratios for igatools can be also found in igatools/utils/astylerc_ansi
#
astyle --style=allman --convert-tabs --indent-preprocessor --indent=spaces=4 \
       --indent-switches --min-conditional-indent=0 --pad-header \
       --unpad-paren --lineend=linux --indent-labels --align-pointer=name \
       --align-reference=name --max-instatement-indent=60 --suffix=none \
       --quiet \
       $(git status | awk -F : '(/modified:/ || /new file:/) && (/\.h\>/ || /\.cpp\>/) {print $2}')

Another snippet demoing astyle is here[4], like:

#!/bin/bash
# Installation:
#   cd my_gitproject
#   wget -O pre-commit.sh http://tinyurl.com/mkovs45
#   ln -s ../../pre-commit.sh .git/hooks/pre-commit
#   chmod +x pre-commit.sh

OPTIONS="-A8 -t8 --lineend=linux"

RETURN=0
ASTYLE=$(which astyle)
if [ $? -ne 0 ]; then
    echo "[!] astyle not installed. Unable to check source file format policy." >&2
    exit 1
fi

FILES=`git diff --cached --name-only --diff-filter=ACMR | grep -E "\.(c|cpp|h)$"`
for FILE in $FILES; do
    $ASTYLE $OPTIONS < $FILE | cmp -s $FILE -
    if [ $? -ne 0 ]; then
        echo "[!] $FILE does not respect the agreed coding style." >&2
        RETURN=1
    fi
done

if [ $RETURN -eq 1 ]; then
    echo "" >&2
    echo "Make sure you have run astyle with the following options:" >&2
    echo $OPTIONS >&2
fi

exit $RETURN

Another project Firmament[5] is more like us, using something like CppLint, however they organize the lint process using Makefile, the script:

#!/bin/sh

echo "Running pre-commit lint pass... (skip with --no-verify)"

git stash -q --keep-index

make lint
RESULT=$?
git stash pop -q

if [ ${RESULT} != 0 ]; then
  echo
  echo "-----------------------------------------------"
  echo "  LINTING FAILED -- please fix above errors.   "
  echo ""
  echo " [Bypass by passing --no-verify to git commit] "
  echo "-----------------------------------------------"
  exit 1
else
  exit 0
fi

Based on the survey, I come up with a strategy which will first filter all the Modified or Added files, then run CppLint on the filtered files(optionally can also run astyle), if bad style found, abort, else pass. The strategy will be implemented using Bash. Of course, to be not so serious, developers can bypass the check by passing --no-verify to git commit.

Any idea? I will try this and keep you informed.

[1] https://code.google.com/p/igatools/wiki/CodingConventions
[2] http://astyle.sourceforge.net/
[3] https://code.google.com/p/igatools/source/browse/utils/pre-commit.astyle
[4] https://gist.github.com/kblomqvist/bb59e781ce3e0006b644
[5] http://www.cl.cam.ac.uk/~ms705/research/firmament/devenv.html

@hustcalm
Copy link
Collaborator Author

@rocapal Got a working git pre-commit hook for running Cpplint here #41 .

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

No branches or pull requests

4 participants