Reynold Requested Review.

[laceysanderson]

Changes: https://github.com/UofS-Pulse-Binfo/rawphenotypes/compare/cb18bae00690eb9ecb4b52a5389d1df43e81387e...master

Check: #3, #4, #5.

[laceysanderson]

Code Review

• [x] include/rawpheno.admin.form.inc, line 943: It's more readable to add to an array using $arr_rows[] = array(); with each element on it's own line.
• [x] include/rawpheno.admin.form.inc, line 955: do not use $query->fetchAll() since it pulls all files into memory. Instead you can iterate over the results directly foreach ($result_F as $f) { (see https://www.drupal.org/docs/7/api/database-api/result-sets).
• [x] include/rawpheno.function.measurements.inc line 429: Using square brackets to denote arrays makes your code require PHP 5.4+ without providing much benefit. You should use array(element1, element2); instead. (You use square brackets more then once... but not always -be sure to be consistent)
• [x] include/rawpheno.upload.excel.inc line 156: You specify that it needs to be in order but you're assuming the plot columns are in the expected order... Have you ensured they are in order elsewhere first?
• [x] include/rawpheno.upload.form.inc line 801: Max execution time: 20 minutes !?! How long is this process actually taking? Perhaps we should consider moving file validation into a separate step run as a tripal job?
• [x] include/rawpheno.upload.form.inc line 801: Also, remember that this hook is called for more then just your module. Thus you have increased the execution time for all file validation on KnowPulse...
• [x] rawpheno.module line 257: Seeing hook_preprocess() is always a red flag for me since it can so easily add variables to every page and is inefficient (called for every page). While I see you are sharing variables amongst a number of hooks which is why you went this route, I think you should duplicate code in this case and use the specific hooks instead. Thus define each of rawphenopreprocess rawpheno_rawdata(), rawpheno_preprocess_rawpheno_instructions(), rawpheno_preprocess_rawpheno_upload_form_master(), rawpheno_preprocess_rawpheno_backup(), rawpheno_preprocess_rawpheno_download() rather then using if statements within rawpheno_preprocess().
• [x] rawpheno.module line 807: Always use readable parameters in $_GET (e.g. rather then p and t use project_id and trait_id).
• [x] theme/css/rawpheno.admin.style.css: Try not to use !important. You can almost always accomplish the same thing by using more selective CSS rules. Using !important makes it more difficult for others to override your styles which makes your module less friendly for sharing.

[laceysanderson]

Functionality: Reviewers Checklist

• [x] Issue #4 and #5 are confirmed fixed.
• [x] Issue #3 still needs to be tested
• [x] File display in project administration seems good. Still need to confirm in Dev/Fresh
• [x] Confirm new version works well in Dev/Fresh (rather then testing in Dev/Reynold)
• [x] Confirm categorize by location/year is working as expected

[laceysanderson]

I tested all the new functionality through the interface and it performs as expected 👍

My only comment from a non-code based perspective is that both the upload and download Tripal jobs print excessively to the screen. This greatly slows down the job. I would print an update at most every 10% and otherwise I would only print errors. Upload is printing for every line and even download is printing ~3 times per 5% (looks like a rounding error).

[laceysanderson]

Back to you @reynoldtan for improvements.

[laceysanderson]

Changes are merged into the main branch 👍