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

Clean up code to match style guide #7

Open thomthom opened 10 years ago

thomthom commented 10 years ago

https://github.com/styleguide/ruby

johnwmcc commented 10 years ago

I've now done this as far as I can take it.

See 'Style cleanup.txt' in su_shapes folder, and newly committed code, for what I've done. I need help on a couple of points, marked with \ in the txt file.

John

thomthom commented 10 years ago

That sounds very good. You can just submit what you have, and then we'll go through the remaining ones. You can create issues of the things you're unclear or. I can help you or I could do the fix.

However, I don't think we should have that file in the repository. The repository itself contain the history of everything done. One see who changed what in the commit history and the commit messages fill in the details. And the pull requests are the method of reviewing what is done. You can make pull requests even if you are not done, just to get feedback.

As for the strings vs symbols, a very detailed article is: http://www.robertsosinski.com/2009/01/11/the-difference-between-ruby-symbols-and-strings/ Basically, one would normally use symbols instead of strings for things like keys in hashes.

for vs each loop:

for face in faces
  # ...
end

becomes

faces.each { |face|
  # ...
}

Note that many times there are other iterator methods available in Ruby that fits better. Depending on what you do with the collection.

johnwmcc commented 10 years ago

I'm unclear whether this for loop (and another one very like it) can be replaced by another construct:

  for i in 0..n90 do
    angle = delta * i
    cosa = Math.cos(angle)
    sina = Math.sin(angle)
    arcpts.push(Geom::Point3d.new(radius*cosa, 0, radius*sina))
  end

This isn't iterating over a collection.

The other iteration you thought could be improved is in the Draw tube class:

 # Draw tube
  outer = container.add_circle ORIGIN, Z_AXIS, outer_radius, num_segments
  face = container.add_face outer
  inner = container.add_circle ORIGIN, Z_AXIS, inner_radius, num_segments
  inner[0].faces.each { |f| f.erase! if(f != face) }
  height = -height if face.normal.dot(Z_AXIS) < 0.0
  face.pushpull height

Since I don't really understand what this is doing, I don't know how to improve it. (is it drawing the end(s) of the inner cylinder?) This is another part of the code that I just used from the 2004 SU original.

You implied, I think, that one could put the 'faces' that aren't really faces into a collection, then erase the collection.

Could it stand?

If not, could you suggest how to replace it?

Best wishes

John

thomthom commented 10 years ago

You don't have to clean up everything in one go. You can just clean what you feel comfortable with. If you see stuff that you think should be cleaned up you can just file a new issue and we'll look at it.

johnwmcc commented 10 years ago

I've taken it as far as I can in shapes.rb v1.4.5 (committed in my master). I've deleted file Style cleanup.txt but moved its contents to a new issue no 14 Code cleanup - issues fixed.

Remaining possible cleanup items that I don't know whether or how to deal with are now posted as separate issues.