[GenABEL-dev] [Genabel-commits] r1892 - pkg/MultiABEL/R

[L.C. Karssen]

Hi Xia,

Thanks for the update to MultiABEL. If I remember correctly, this is the
first time I do a commit review for you.
In the GenABEL project we try to do quick reviews of all code that is
committed. In the review we look both at coding style and at the effect
of the code (Does it do what it is supposed to do? Are there any side
effects? Etc.).
Feel free to help out by subscribing to the GenABEL commit mailing list
(if you haven't already done so).

Please find my comments below.

On 26-11-14 06:51, noreply at r-forge.r-project.org wrote:
> Author: shenxia
> Date: 2014-11-26 06:51:01 +0100 (Wed, 26 Nov 2014)
> New Revision: 1892
> 
> Modified:
>    pkg/MultiABEL/R/Multivariate.R
> Log:

Next time, could you please add a log message to your commit? The idea
of the log message is that it briefly explains what the change does/is
supposed to do, which bug is fixed, which (partial) feature added, etc.

> 
> 
> Modified: pkg/MultiABEL/R/Multivariate.R
> ===================================================================
> --- pkg/MultiABEL/R/Multivariate.R	2014-11-18 12:13:25 UTC (rev 1891)
> +++ pkg/MultiABEL/R/Multivariate.R	2014-11-26 05:51:01 UTC (rev 1892)
> @@ -68,7 +68,7 @@
>          GenABEL <- FALSE
>          cat(' OK\n')
>      } else if (!is.null(gwaa.data)) {
> -        pheno <- gwaa.data at phdata
> +        if (!is.null(phenofile)) pheno <- read.table(phenofile, header = TRUE) else pheno <- gwaa.data at phdata

The line above is rather long. This doesn't help making the code easily
readable. Moreover, since it is an if clause it is better to add {}s to
make it 100% clear what is supposed to be in the if and else parts. For
this statement the intention is quite clear, but imagine someone adds a
debug print statement somewhere, or wants to quickly add something in
the if statement. Having the curly braces is good practice (and part of
our coding standards).


Thanks again for your time and efforts,

Lennart.

>          GenABEL <- TRUE
>          cat(' OK\n')
>      } else {
> 
> _______________________________________________
> Genabel-commits mailing list
> Genabel-commits at lists.r-forge.r-project.org
> https://lists.r-forge.r-project.org/cgi-bin/mailman/listinfo/genabel-commits
> 

-- 
*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*
L.C. Karssen
Utrecht
The Netherlands

lennart at karssen.org
http://blog.karssen.org
GPG key ID: A88F554A
-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 213 bytes
Desc: OpenPGP digital signature
URL: <http://lists.r-forge.r-project.org/pipermail/genabel-devel/attachments/20141126/96150951/attachment.sig>