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

[Yurii Aulchenko]

one comment below

> On Nov 26, 2014, at 14:02 PM, L.C. Karssen <lennart at karssen.org> wrote:
> 
> 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

Better use phdata(object) method than direct access to slot. This should make your code more stable to potential changes in underlying class in the future. 

best,
Yurii

> 
> 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
> -*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-
> 
> _______________________________________________
> genabel-devel mailing list
> genabel-devel at lists.r-forge.r-project.org
> https://lists.r-forge.r-project.org/cgi-bin/mailman/listinfo/genabel-devel