Re: Contributing code

[Carnë Draug]

On 15 December 2012 16:00, Roberto Metere <address@hidden> wrote:
> On 12/14/2012 12:58 PM, Juan Pablo Carbajal wrote:
>
> On Fri, Dec 14, 2012 at 12:36 PM, Roberto Metere <address@hidden> wrote:
>
> Dear Octave maintainers,
>     I would like to contribute octave-forge repository by adding a missing
> function "strel" to the package image, which I have implemented for the most
> used arguments. Some cases don't work yet (exiting with a "not yet
> implemented" message), but I surely will keep on work.
>
> My SourceForge name is trinitrina
> I have attached my contribution.
>
> I may imagine you have some rules for coding, like no tabs, custom comments,
> etc.
> Obviously I want to change my code to satisfy those rules, may someone help
> me to begin?
>
> Best regards,
>     Roberto Metere
>
> Roberto,
>
> Thank you fro the contribution!
>
> the best way to get to know the standard is to look at other files
> already in the image package.
> You can also check the guidelines here
> http://www.gnu.org/software/octave/doc/interpreter/General-Guidelines.html
>
> Also follow this thread
> https://mailman.cae.wisc.edu/pipermail/octave-maintainers/2010-November/021419.html
>
> Juan,
>     As first, thank you for your previous mail.
> I adapted my code to respect general guidelines and added two initial test
> cases at the end, following what I have seen in other files in "image"
> package.
>
> I think it's time for my first "Initial commit to SVN."
> How can I do? do I need permission, don't I?
>
> What I've just done:
>
> I launched "svn checkout
> http://svn.code.sf.net/p/octave/code/trunk/octave-forge/";
> Added my file to main/image/inst/

Hi Roberto

We usually request new contributors for a few contributions before
giving them commit access. I'll happily commit your code and
eventually we will give you access.

I have taken a look at your new code and here's a list of improvements
you could do:

1) matlab strel does not return a logical matrix, its "getnhood" method does.

2) there's other simpler changes that would make the code much simpler
to maintain. As an example take a look at
http://agora.octave.org/snippet/q2hq/ (the first block is yours, the
second is an improvement, the third is what is usually into Octave).
  * too many nested if blocks. You can use ! before the test and elseif
  * while it's good to be specific on the error message, on this case
should be enough to place them all under the same error message
  * don't use size (x) == [1 1] to check if it's a scalar. Use isscalar()
  * don't use uint8 to check if a number is an integer, that would
place a limit on 255. Use fix()
  * don't use [ ] around quotes. They are not necessary. If you want
to add a variable to it, use the syntax "error ("the thing %s is not
implemented", varname)"
  * don't use "ones (x, x, "logical")", use "true (x)" instead
  * since we already made the check that arg1 is a single positive
integer, you can use it directly, no need for uint8()
  * the error message I used at the end is just to be standard

Such change could be easily applied to the rest of your code.

Again, please make further code submission to the feature request
tracker at https://sourceforge.net/p/octave/feature-requests/ They are
less likely to get lost there and the maintainers mailing list is not
the place to discuss this things.

Carnë