SketchUp / sketchup-shapes

Shapes adds new tools for drawing more shapes and primitives in SketchUp.
http://extensions.sketchup.com/content/shapes
MIT License
26 stars 13 forks source link

Code cleanup - issues fixed #14

Closed johnwmcc closed 10 years ago

johnwmcc commented 10 years ago

I've been through the GitHub Ruby Style Guide and cleaned up all the code I know how to.

I've raised separate issues for the ones I don't know how to deal with, marked with \ in the list below:

Classes

Avoid the usage of class (@@) variables due to their unusual behaviour in inheritance - used extensively, but not with inheritance \ I don't know how else to get variables persistent across uses of the plugin

Use def self.method to define singleton methods - two instances. I don't see any other opportunities to use this

Avoid class << self except when necessary - not used

Indent the public, protected, and private methods as much the method definitions they apply to. Leave one blank line above them - not used

Avoid explicit use of self as the recipient of internal class or instance messages unless to specify a method shadowed by a variable - not used

Collections

Prefer %w to the literal array syntax when you need an array of strings - no string arrays used

Use Set instead of Array when dealing with unique elements - no relevant usage

Use symbols instead of strings as hash keys \ I think these have to be strings, as they are reassigned new values for each new instance of a Class

Strings

Prefer string interpolation instead of string concatenation - neither used

Prefer double-quoted strings - all single quoted strings changed to double

Avoid using String+ when you need to construct large data chunks. Instead, use String<< - neither construct used

Regular Expressions

Not used

Percent literals

Not used

Hashes

Use hashrocket syntax for Hash literals - changed in defaults = {...} lines

2014-02-08 v1.4.5

Earlier issues revisited from issue #3 list and fixed

Use unless file_loaded(__FILE__) instead of if (not $shapes_menu_loaded) - changed (note, needs to be file_loaded?(__FILE__) with question mark to work)

No parentheses in case(length_unit) or case(key) - now all removed

Is this f.erase! safe? Erasing from the collection it iterates might be problematic. It's normally safer to collect entities to erase and bulk erase them after iterating. It's also faster to erase in bulk than individual entities \ That may be so, but I don't know how to do that. And there are so few faces that speed doesn't matter here

Logic error and change in defaults for Dome and Sphere:

Fixed logic error in defining default number of segments for Dome and Sphere, (which was accidentally still hard coded to 5 in the defaults = {...} line)

Changed default number from 5 to 4 per 90 degrees for consistency with base circle segments for Cone, Cylinder, Tube and Torus

johnwmcc commented 10 years ago

Sorry, closed issue by accident

thomthom commented 10 years ago

That's __FILE__ and not FILE.

johnwmcc commented 10 years ago

That's what I typed, and what's in the revised code. But I think Markdown converted it to bold instead, since I forgot to enclose it in code backticks. Now corrected in the comment above.

WRT Dome and Sphere, the code is in their Class create_entities - around lines 679 and 761