SketchUp / sketchup-stl

A SketchUp Ruby Extension that adds STL (STereoLithography) file format import and export.
http://extensions.sketchup.com/content/sketchup-stl
MIT License
246 stars 68 forks source link

Class instance variables? #91

Open jimfoltz opened 10 years ago

jimfoltz commented 10 years ago

I'm not sure I like how I use class instance variables in some of the methods. For example, the @mesh_file variable which shows up in several methods. It's used more or less like a global variable, and is in fact "global" to the Exporter scope. The way it just pops up in methods is a little jarring. The more I look at it, the less I like it.

So what is a better way to write the method? Should I add a file argument to each of the methods that use @mesh_file?

thomthom commented 10 years ago

Yea, I think that might be useful. I think that the less instance variables the methods use the easier they are to unit test as well. I've also found it generally easier to follow the code flow with less instance variables.

briangbrown commented 10 years ago

You can always just add a get_file method to provide yourself a seam for testing if you don't want to add a file param to each method. It is a bit weird that the methods that write to @mesh_file assume it is in a certain state and even close it as a byproduct of writing the footer. That said, this isn't Java, so you can always change @mesh_file to anything at any point in a test.

I do like the way you setup the @write_face method. It would be nice to do the same for write_header and write_footer. It would make adding a new format easy (though there may never be another).

thomthom commented 10 years ago

We do have another project started, PLY. We're reusing quite a bit of the structure from STL.

There have also been talks about AMF format in the issues. The more flexible and reusable we can make the importer, exporter project the better it would be for future projects. We could in fact create a boiler plate template. Something that would be very nice once we get test units up and running.

thomthom commented 10 years ago

Is this still valid?

jimfoltz commented 10 years ago

The write_face method is still being stored in a @write_face variable, so still open.