Re: plot templates and options lists for set, plot etc.

[John W. Eaton]

On 16-Sep-2009, Thorsten Meyer wrote:

| Thorsten Meyer wrote:
| > Here is a patch implementing struct and cell array arguments for the set 
function.
| > 
| > I have impplemented the matlab behaviour (as I understand it ...). See the 
tests
| >  added to the patch for the details.
| > 
| > Could somebody please review the patch (what I do in the sources is still a 
lot
| > of trial and error):
| >  - Have I got the types of the various variables right?

| >  - Is it ok to add doxygen style comments to the new methods? (the
| >    doxygen docs

I don't use Doxygen, so I'm unlikely to remember to add the markup in
the comments I write.  If we decide to do this, then I think we should
try to do it on a more global scale.

| >  - What about the coding style?
| >  - Are the added methods for the graphics_object ok (also where I placed the
| > definitions)?

I try to keep the order of the declarations in the .h file the same as
the order in the .cc file.  If you've done that, then it is probably
OK.

| +//! Set properties given in two cell arrays containing names and values.
| +void
| +graphics_object::set (const Array<std::string> names,
| +                      const Cell values, octave_idx_type row)

I think this should be 

  graphics_object::set (const Array<std::string>& names,
                        const Cell& values, octave_idx_type row)

| +  if (! (names.numel () == values.columns ())) 

Why use

  ! (x == y)

instead of

  x != y

?

| +void
| +graphics_object::set (const Octave_map m)

This should be

  void
  graphics_object::set (const Octave_map& m)

| +//! Set a property to a value or to its (factory) default value.
| +void graphics_object::set_value_or_default (caseless_str& name,
| +                                            octave_value& val)

I think this should be

  void
  graphics_object::set_value_or_default (const caseless_str& name,
                                         const octave_value& val)

unless the function needs to modify NAME and VAL.  It doesn't appear
need to.

| +  if (val.is_string ())
| +    {
| +      caseless_str tval = val.string_value ();
| +
| +      if (tval.compare ("default"))
| +        val = get_default (name);
| +      else if (tval.compare ("factory"))
| +        val = get_factory_default (name);
| +    }
| +
| +  if (error_state)
| +    return;
| +
| +  rep->set (name, val);
| +}

With the non-const VAL, if VAL is a string, then it will also be
changed in the caller.  Is that really what you want?  I think it
would be better to define a local temporary value inside the
function.

jwe