openscad / MCAD

OpenSCAD Parametric CAD Library (LGPL 2.1)
http://reprap.org/wiki/MCAD
578 stars 192 forks source link

Updating coding style of 2D shapes #35

Closed Irev-Dev closed 4 years ago

Irev-Dev commented 6 years ago

Hi Loong Jin,

After we spoke about some coding style rules I kept thinking of more, and instead of asking you about each one I just went ahead and implemented my own rules on one file and stated them below. Please let me know which ones you want changed.

Unfortunately this doesn't review nicely with github, it seems to not recognise line modifications where indentations have change, and instead shows whole blocks removed and added. It must have something to do with tabs being replaced with spaces!? I also had trouble where the indentation looked correct on my editor (VSCode) but when pushed to github it was wrong again. I fixed it by redoing every single line's indentation, I'll have to come up with a better solution in future.

The examples all still worked when I finished. So I don't think I broke anything.

Preliminary code rules:

Irev-Dev commented 6 years ago

Is the problem I mentioned about github not recognising individual changes with indentation going to be a problem when it comes to merging other branches? and therefore should updating the code style be left to much later, when the repo is much more tidy?

kintel commented 6 years ago

I think official guidelines for code written in the OpenSCAD language makes a lot of sense, at least for code and libraries maintained and endorsed by the extended OpenSCAD team. Some initial comments:

variable_names should use underscores between words I feel there are too many schools of thought here. I tend to use mixedCase for variable names myself. The key is to try to be consistent within one script.

ClassNames should use camel case starting with a capital. I assume this one is copy pasta from some other guidelines?

moduleNames should use camel case NOT starting with a capital. I tend to think of modules as "classes" in other languages and hence use CamelCase for them. For built-in modules, we use snake_case. This differentiation is very similar to how the C++ standard library does it.

New line for each child in a chain with no extra indentation This is a bit unclear to me. Are you talking about newlines caused by too long lines, or chains in general? I find long chains to be tricky to get right, stylewise, in OpenSCAD. I like constructs like this:


rotate(...) translate(...) object(); // Single-line statement
if (expression) someObject(); // Single-line if

if (expr) { anotherObject(); // Multi-line if: Always use curly braces for grouping }

// Multi-line single-expression chain. Indentation helps see where it ends without using curly braces for grouping rotate(someVeryLongArgument) translate(anotherVeryLongArgument) MyObject(yetAnotherVeryLongArgument);



Note: There is no penalty for using curly braces to group "multi-line single-expression" chains, but I also don't find the result to be very readable.

> blank line between each chain
I find it hard to enforce blank line rules. I think blank lines should be used to separate blocks of code that has a natural/logical grouping. One line per chain sounds like a lot of space.

> new line for curly braces '{' except for module definitions
I would argue that the same rule can be used everywhere. If anything, I tend to do the opposite: Don't use newlines before `{` unless there is a good reason to (e.g. for large modules where it makes code stand out more.

> examples to be wrapped in example module
This will cause the example module to be exported into the global namespace. This may not be the intention behind the examples.

> Max line length of 80
A bit old school. Most styles I see these days have moved to ~120.
Since OpenSCAD tends to produce long lines, this makes life a bit easier.
hyperair commented 6 years ago

On Fri, Feb 23, 2018 at 06:14:46AM -0800, Marius Kintel wrote:

I think official guidelines for code written in the OpenSCAD language makes a lot of sense, at least for code and libraries maintained and endorsed by the extended OpenSCAD team.

Some initial comments:

variable_names should use underscores between words I feel there are too many schools of thought here. I tend to use mixedCase for variable names myself. The key is to try to be consistent within one script.

There are many schools of thought here, but I feel that we should be consistent within the project as well.

ClassNames should use camel case starting with a capital. I assume this one is copy pasta from some other guidelines?

Not really. When I described my initial thoughts, it for data-structure-ish things, e.g. https://github.com/hyperair/deltabob/blob/vslot-upgrade/lib/delta.scad

moduleNames should use camel case NOT starting with a capital. I tend to think of modules as "classes" in other languages and hence use CamelCase for them. For built-in modules, we use snake_case. This differentiation is very similar to how the C++ standard library does it.

I thought of modules as functions, and functions as... functions as well. I prefer to use snake_case in this case actually.

https://github.com/openscad/MCAD/blob/dev/array/translations.scad

New line for each child in a chain with no extra indentation This is a bit unclear to me. Are you talking about newlines caused by too long lines, or chains in general? I find long chains to be tricky to get right, stylewise, in OpenSCAD. I like constructs like this:


rotate(...) translate(...) object(); // Single-line statement
if (expression) someObject(); // Single-line if

if (expr) { anotherObject(); // Multi-line if: Always use curly braces for grouping }

// Multi-line single-expression chain. Indentation helps see where it ends without using curly braces for grouping rotate(someVeryLongArgument) translate(anotherVeryLongArgument) MyObject(yetAnotherVeryLongArgument);



Note: There is no penalty for using curly braces to group "multi-line
single-expression" chains, but I also don't find the result to be very
readable.

I really meant:

module blah ()
{
    parent ()
    parent ()
    child ();

    parent ()
    parent ()
    child ();
}

This helps keep the indentation for long single-child transformation chains from going out of hand. I do find your style agreeable too though.

blank line between each chain I find it hard to enforce blank line rules. I think blank lines should be used to separate blocks of code that has a natural/logical grouping. One line per chain sounds like a lot of space.

It's only a lot of space if you have very sparse transformation chains.

new line for curly braces '{' except for module definitions I would argue that the same rule can be used everywhere. If anything, I tend to do the opposite: Don't use newlines before { unless there is a good reason to (e.g. for large modules where it makes code stand out more.

I was following K&R in this regard, which I think is quite widely adopted in C++ as well.

examples to be wrapped in example module This will cause the example module to be exported into the global namespace. This may not be the intention behind the examples.

Maybe we should throw the examples into a separate directory altogether. /examples maybe?

Max line length of 80 A bit old school. Most styles I see these days have moved to ~120. Since OpenSCAD tends to produce long lines, this makes life a bit easier.

120 is fine too. I prefer 80, but don't have very strong feelings against 120.

-- Kind regards, Loong Jin

hyperair commented 6 years ago

Unfortunately this doesn't review nicely with github, it seems to not recognise line modifications where indentations have change, and instead shows whole blocks removed and added. It must have something to do with tabs being replaced with spaces!? I also had trouble where the indentation looked correct on my editor (VSCode) but when pushed to github it was wrong again. I fixed it by redoing every single line's indentation, I'll have to come up with a better solution in future.

I haven't had time to look at this properly yet, but it's normal for whole-file indentation changes to look like entire blocks were modified.