[evla-sw-discuss] [Fwd: Re: Code Review]

[Bryan Butler]

i'm trying this again, as it bounced last time :/...

	-bryan



-------- Original Message --------
Subject: Re: Code Review
Date: Tue, 20 Jun 2006 14:12:45 -0600
From: Bryan Butler <bbutler at nrao.edu>
To: evla-sw-discuss <evla-sw-discuss at donar.cv.nrao.edu>
References: <4461F8D0.2010909 at aoc.nrao.edu>


i found my notes for this review at the bottom of my backpack this afternoon, so
am transcribing them for posterity.  mea culpa for not sending them out earlier.

	-bryan




Review Team
============
Author: Barry Clark
Chairperson: H. Ben Frej (standing in for P Whiteis, who had a conflict)
Scribe: B Butler (standing in for J Benson, who didn't show up)
Lector: Dave Harland
Member: K. Sowinski


Code of Interest
============
Module:  Subarray.java

A fairly long (337 lines) module, but much of it is boring code to
take a message and ship it off to all the Antennas who belong to the
Subarray.

Source:
file:/home/mchost/bclark/NRAO/EVLA/OBSERVE/target/docs/xref/index.html
and click on Subarray

Javadoc API:
file:/home/mchost/bclark/NRAO/EVLA/OBSERVE/target/docs/apidocs/index.html


Preparation
============
HBF - 1.5h; DH - 1.5h; BGC - 1h; BJB - 0.5h (serendipitously); KS - 0h


Context
============
. Subarray created at script startup
. antennas are supported with AntennaPhysical
. Subarray mostly allows for things to be done over multiple antennas,
   but also allows for adding and removing antennas from real subarrays


Notes of Interest
============
. line 55 - method addAntenna - DH points out that this is the place that
   you should make sure that a new antenna that gets added to a subarray
   gets initialized correctly - like focus, wrap, etc.  BGC agrees (says
   it is 'food for thought').

. line 103 - method setLoIfSetup - DH points out that Java 5.0 allows
   enumerations to be done much more easily.  similar comments apply
   throughout, where similar looping constructs exist.  BGC agrees, and
   comments that those loops are 'fossils'.

. lines 136 & 139 - method setSource - naming the Source as "lo" is clearly
   a cut-and-paste error.  BGC agrees.

. line 188 - method restoreInterferometerModel - BJB points out that the
   comments for this method should make clear the fact that this restores
   the model to CALC explicitly.  BGC agrees.

. line 246 - method registerPointing - BJB points out that you should
   probably call registerPointing for each.  BGC agrees.

. line 283 - method execute - DH queries on units of time.  BGC answers
   days.  all note that this should be added to comments.

. lines 292,293 - method execute - all point out that 1/86400 should
   be added as a constant rather than written explicitly.

. lines 294-297 - method execute - DH queries as to why the sequence
   is precalculateA, calculateA, calculateA, executeA, and why those
   particular values of time are used (ttime - 0.5*deltaTime, ttime
   + 0.5*deltaTime, ttime + 1.5*deltaTime)?  BGC explains, but all
   agree that there needs to be some commenting of this.  BJB points
   out that it might be better (more understandable) to split it into
   a calcInitialize, calculate, calculate, calculate sequence, so that
   it's more clear what is doing what (since precalculate just does an
   initialize of CALC, then a call to calculate).  BGC seems unconvinced
   on this latter.

. lines 305, 307 - method execute - BJB points out that these println
   statements need to go to the logger instead of just being printed.
   BGC agrees.  also, 'interupted' -> 'interrupted' (spelling error).





On 5/10/06 08:29, Pete Whiteis wrote:
> Reminder: Code Review this morning 10:00, CR 152.   My son is ill today 
> and I'll need to head home shortly.   Hichem has generously agreed to 
> stand in for me as chairman.