[Octave-patch-tracker] [patch #7929] Partial Differential Equation Solver in two dimensions

[Jordi Gutiérrez Hermoso]

Follow-up Comment #7, patch #7929 (project octave):

Okay, so I'm starting to think about importing your code into Octave...

I won't comment on the mathematical aspects of it, so please be patient with
the apparently trivial remarks I have to make about its superficial
appearance.

Why are you putting 1; at the top of files, in effect using script files and
no function files?

Should this be a package or a core function?

There are many stylistic issues to also discuss. We prefer a different Octave
house style to Matlab style. Read some of Octave's source code to glean this
style and this:

   
http://www.gnu.org/software/octave/doc/interpreter/Octave-Sources-_0028m_002dfiles_0029.html

I see the following problems with your code:

* Using inline("foo(a,b)", "a", "a") instead of @(a,b) foo(a,b)
* No explicit end statements (endfor, endif, endwhile...)
* % for comments instead of ##
* No space between function names and opening bracket: sin (x), not sin(x)
(but note, no space for indexing, just a visual cue to distinguish function
calls from indexing).
* Single-quoted strings. Except in some cases, we generally prefer
double-quoted strings.
* Exceedingly long lines. Lines should not be more than 80 characters wide.
Note that unlike Matlab, Octave does not require continuation characters if
there's an opening bracket that hasn't been closed on this line yet. Use this
feature to your advantage when breaking up lines.
* Using ~= instead of the more common !=
* No spaces after commas.
* Inconsistent use of tabs and indentation. Just don't use any tabs at all and
use two-space indentation.
* Windows newlines (/r/n). We use Unix newlines (just /n).

Here is a more profound problem: I see a lot of code duplication here. Code
duplication is very bad. In the future as we need to maintain this code, code
duplication means that we have to hunt down all instances of the same code and
patch it in the same way. Please move as much common code as you can into
common functions shared by others. If necessary, use private directories and
or subfunctions. 

Another problem, I see some very trivial loops that can easily be replaced.
Instead of


function [u] = osb22ASol( x , t)
    C1 = 0.1;
    C2 = 1.0;
    P = 1.1;
    for i=1:length(t)
        for j=1:length(x)
            if x(j)<0
                u(i,j) = log( C1 * x(j) + t(i) + P);
            else
                u(i,j) = log( C2 * x(j) + t(i) + P);
            end
        end
    end
end


this could easily be


function [u] = osb22ASol(x , t)
  P = 1.1;
  C = ifelse (x < 0, 0.1, 0.2);
  u = log (C*x(:) + t(:)' + P)
endfunction


This is just from a quick glance of the code, sorry I didn't delve deeper into
the actual mathematics of it.

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/patch/?7929>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/