3
\$\begingroup\$
procedure E(n, m: integer; A, B: tMatrix; var C: tMatrix);
var i, j: integer;  
begin  
  for i:=1 to n do  
    for j:=1 to m do begin  
      if i<=j then C[i,j]:=A[i,j] + B[i,j] else C[i,j]:=A[i,j] - B[i,j];  
    end;  
end;

Input: three matrix. Output: new matrix which created by addition or subtraction of two elements of two matrix depending on conditions

\$\endgroup\$
  • 2
    \$\begingroup\$ This question needs more context. There is no new matrix created... you are reusing the third matrix, and what is the condition? Just that i <= j? \$\endgroup\$ – rolfl May 15 '15 at 17:48
3
\$\begingroup\$

You are only using single-letter variable names. While this is quite understandable for A, B and C, I don't recommend it for n and m.

The big problem about n and m (and i and j) is that it is hard to tell what is the row and what is the column. (The thing I hate the most about mathematics is the undescriptive variable names, it is not something I'd personally recommend carrying over to the programming side of things)

Speaking of names, your method is named E. That really doesn't say anything whatsoever.

I believe it's convention in Delphi to use an uppercase letter for the T prefix, so that would be TMatrix.

I'd use slightly more spacing in the code, and I'd also use a begin also for the outer for-loop.

This line:

if i<=j then C[i,j]:=A[i,j] + B[i,j] else C[i,j]:=A[i,j] - B[i,j]; 

is, except for the short and confusing variable names, not that bad really. I think it boils down to opinions how to write it 'the best'. An alternative to what you are doing is to use a addValue variable:

procedure ConditionallyAdd(rows, cols: Integer; A, B: TMatrix; var C: TMatrix);
var
  row, col: Integer;
  addValue: Real; // or Integer? or something. The type that your matrix uses
begin
  for row := 1 to rows do begin
    for col := 1 to cols do begin
      if row <= col then addValue = B[row, col];
      else addValue = -B[row, col];
      C[row, col] := addValue;
    end;  
  end;
end;

There is essentially a lot of different ways to write this. Use the way you like the best.


I suspect that the integer values passed to the procedure always match the size of the matrices, in that case, use rows and cols as properties of the matrices if possible.

\$\endgroup\$
1
\$\begingroup\$

Overall comments:

  • No comments!
    You should comment some of your code. You should always have a function-level comment that describes what function does. While the code itself should be self-documenting as much as possible, that function-level comment is important. It describes intent. You should also comment non-obvious parts of the code.
  • No blank lines.
    Whitespace shows chunks of code.
  • Use of single character names.
    Sometimes this is okay, but more often than not it isn't. I don't like i and j as loop variables; to many false positives when searching (particularly with i). At a minimum I use variables such as ii, jj, kk, but when dealing with 2D arrays, I tend to use irow and icon as loop indices.
  • if/then/else all on one line.
    This makes for long lines that hide the logic and operation of the code. This style can also present problems for debuggers. Many debuggers don't step into the then or else when the statement is all on the one line.
  • Decision statement inside a doubly nested loop (potentially slow performance).
    This might be classified as speculative generality, but I always tend to split code such as this into two loops, one for the lower left portion of the matrix and the other for the upper right portion. There are a surprisingly large number of numerical algorithms that do something different for the lower left and upper right portions of a matrix. The code is always split in parts.

Putting all of this together,

{ Forms the matrix C as the sum/difference of the matrices A and B.

  The upper right triangular sub matrix of C (including diagonal elements)
  contains the sum of the corresponding elements of A and B while the
  lower left triangular sub matrix of C (excluding diagonal elements)
  contains the difference between the corresponding elements of A and B.

  @param nrows (Number of rows in matrices A, B, and C)
  @param ncols (Number of columns in matrices A, B, and C)
  @param A (Input matrix; augend or minuend)
  @param B (Input matrix; addend or subtrahend)
  @param C (Output matrix; sum or difference)
}
procedure splitAddSubtract (nrows, ncols: integer; A, B: tMatrix; var C: tMatrix);
  var irow, icol: Integer;
begin

  for irow := 1 to nrows do begin

    { Lower left submatrix: Construct difference between A and B}
    for icol := 1 to irow-1 do
      C[irow,icol] := A[irow,icol] - B[irow,icol];

    { Upper right submatrix: Construct sum of A and B}
    for icol := irow to ncols do
      C[irow,icol] := A[irow,icol] + B[irow,icol];
  end

end
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.