Deep Survey

I read through all the bullet points but don't feel like I saw any that are actually different from Webform.

I don't know Quiz or Surveys so I can't speak to overlap with them. Can you give more details on the differences in the hope it will help me understand?

Subscribing

* git release branch is wrongly named, see http://drupal.org/node/1015226
* Remove all old CVS $Id tags, not needed anymore
* lines in README.txt should not exceed 80 characters
* "//register survey with itself" comments should start capitalized and end with a ".", there should be a space after "//".
* Run the coder module on your files, there are some doc block formatting errors http://drupal.org/project/coder
* do not use "/** ... */" style comments in function bodies, use "//" to distinct them from function doc blocks

Wrong branch name, 6.x should be 6.x-1.x.

Review of the 6.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• Comments: there should be a space after "//".
  

  deepsurvey_items/deepsurvey_question/deepsurvey_question.module:425:  //check for filtering
  deepsurvey_items/deepsurvey_question/deepsurvey_question.module:918:      //seed the random number generator
  deepsurvey_items/deepsurvey_question/deepsurvey_question.module:1094:  //set up next question
  ... many other places ...
  

• ./deepsurvey_items/deepsurvey_question/deepsurvey_question.admin.inc: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

    // Add code to determine if the node is a survey related one or not - go to the node edit page if it is not a survey - add new db table to store this info.
  

• ./deepsurvey_items/deepsurvey_question/deepsurvey_question.module: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

    // Eventually this function will have to clean up/archive data and other survey related tables.
    // Answer list headings - used currently for validating single response axes but will form actual headings eventually.
    ... many other places ...
  

• Comments should be on a separate line before the code line, see http://drupal.org/node/1354#inline
  

  ./deepsurvey_items/deepsurvey_question/deepsurvey_question.module:650:  $single_response_flag_row = FALSE; //lets us know if the foreach is currently porcessing a single response list - which is handled differently to the other types
  ./deepsurvey_items/deepsurvey_question/deepsurvey_question.module:907:  $current_code; // Used to check for duplicate codes.
  ./deepsurvey_items/deepsurvey_question/deepsurvey_question.module:1221:  $order = 0; // Counts the loops and hence the order of the items.
  ... many other places ...
  

• ./deepsurvey_items/deepsurvey_question/deepsurvey_question.module: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  
  function deepsurvey_question_deepsurvey_item_form($form_state, $instance, $survey, $usr, $parent_item, $parent_type, $loops, $state_values, $state_values_global) {
  --
  
  function deepsurvey_question_deepsurvey_item_form_build_question($instance, $survey, $usr, $parent_item, $parent_type, $loops, &$state_values, &$state_values_global) {
  --
  
  function _deepsurvey_question_get_axis_array($current_question, $axis, $seed_value, $rot_value, &$heading_boundaries) {
  ... many other places ...
  

• ./deepsurvey_items/deepsurvey_question/deepsurvey_question.module: The description for the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  1459- *  If $order is 0 then this is the node id of the survey object the node(s) is being added to, otherwise it is the item/survey relationship that the nodes are to be inserted after.
  1462- *  The order of the node before the new node(s) or the value of the next relationship id if order isn't 0.
  1465- *  The current path to this page, used to build links to the child nodes.
  ... many other places ...
  

• Assignments should have a space before and after the operator, see http://drupal.org/node/318#operators
  

  ./deepsurvey_items/deepsurvey_question/deepsurvey_question.module:927:      for ($itr=0; $itr < $rot_value; $itr++) {
  ./deepsurvey_items/deepsurvey_question/deepsurvey_question.module:972:    for ($itr=0; $itr < $rot_value; $itr++) {
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

manual review:

• remove all debug statements and code lines that are commented out.
• all comments should start capitalized and end with a "."

Please remove the 6.x branch

git push origin :6.x
git branch -d 6.x

See http://ventral.org/pareview/httpgitdrupalorgsandboxchrisp1136296git

you don't have to worry about that, just test through some of the well known full projects and you'll see that most of them are violating the coding standarts.
But anyway we want to be shure that you know about them, you can use them and hopefully you will continue following them.

Switched back to needs review, so in-depth reviews won't be blocked by coding standart issues.

Hello,

I ran your code through pareview and there are many things that you will need to fix. I am in the process of having my module reviewed as well, and trust me they will not consider this project to be accepted until all these items have been fixed.

You can run your code through yourself to make sure all is fixed: http://ventral.org/pareview

Also the code sniffer project though a pain to install is quite worthwhile and will definitely save you time: http://drupal.org/project/drupalcs

I recently got it installed on VIM and Eclipse, and it is making my workflow much much better.

Attached for your convenience is the output from pareview.

Scroll a little down to "Data types on param/return" and have a look at the changes introduced by d8. (Yes, your code should already follow it now)

My advice with the doxygen is every duck must be in a row. If one thing is off it can trigger lots of other messages, and really once you fix the first error it may in fact clear lots of the other others.

I just put your code in my own editor and the first error I notice using drupal code sniffer (which I recommend for this review process: http://drupal.org/project/drupalcs) is your doxygen varialble in line 36 of deepsurvey.admin.inc does not match up with the first variable name in your actual function. The first doxygen variable is @param $deepsurvey and the first actual varialble name in the function is $deepsurvey_item_nid.

Setting up drupal code sniffer as I mention above will greatly enhance your ability to debug all these errors. I am relatively new to this process as well, and this greatly improved my ability to address these issues, and it greatly decreased my frustration with the whole process. It is a bit of a pain to setup but forever after you will be a happier drupaler.

I ran pareview again on your project and there are many many errors still, so make sure you run your code through there and have no errors or the project will not get through the admins to be sure. I try to follow up on the same project I've reviewed previously, and sorry I didn't see your question earlier, I just got back from vacation anyhow.

Cheers, and good luck.

Chris, thanks for your contribution. I know some researchers who might find this useful. I was able to enable your module in 6.25 without issues.

• Thank you for providing documentation; your many screenshots make the use of the tool clear. It may be a small thing, but I would recommend placing your documentation in a subdirectory (./docs or similar).
• The documentation is necessary; at first I found the structure of the relationships between questions, answer lists, and questionnaires confusing. I think others may experience this, too. I have used another custom-built questionnaire tool created by linking content types in a similar way, and our users found it extremely difficult to understand for some reason. They happened to be specialists in performance measurement! Maybe pointing out that one should create everything "backwards," starting with answer lists, would be helpful.
• What have you considered, with respect to a more intuitive creation of a survey with questions and answers? Am I missing something; perhaps you have intended this for very specific applications that aren't immediately apparent? I ask because I find online questionnaire creators such as SurveyMonkey very easy to use, and IIRC, you can re-use questions you create there.
• After creating a new node of the Deep Survey content type, I clicked both the "add a new item to the start of the survey" and "add existing items to the start of the survey" and got 404 errors for both. I couldn't get past that. These were at /admin/deepsurvey/48/new/0 and /admin/deepsurvey/48/existing/0 on my Drupal instance, respectively. Do you know why that might have happened? All the modules were enabled: Answer List, Question, Skip, and Deep Survey.
• I think "altering the flow of the survey" based on the answers to previous questions is a great feature! I think it's important to make that easy for folks to use... the online survey folks have some interesting approaches to this.
• Finally, though this suggestion may not be so welcome, it looks like feedback provided by the PAReview script has continued to improve, and when re-running it on your code I found that you'll get new suggestions there (http://ventral.org/pareview/httpgitdrupalorgsandboxchrisp1136296git).

Chris,

Regarding the 404, I am using a LAMP stack with clean URLs disabled.

It appears that the problem arises when I visit http://localhost/test/drupal-6.25/?q=admin/deepsurvey/48, and the links ("add a new item..." and "add existing items...") are constructed in lines 1044 and 1049 of deepsurvey.module from $current_path. These point to http://localhost/test/drupal-6.25/?q=/test/drupal-6.25/admin/deepsurvey/48/new/0 and http://localhost/test/drupal-6.25/?q=/test/drupal-6.25/admin/deepsurvey/48/existing/0, respectively.

The URL for the Deep Survey node is http://localhost/test/drupal-6.25/?q=admin/deepsurvey/48.

Edit: corrected URLs.

Chris, I edited my comment to correct those URLs. They should all say "6.25" now.

I'm ready to review as soon as you are. :) We really need this.

Hello Chris,

Congrats on this module, it's really impressive and well coded. I simply loved the documentation and how you generated a PDF with screenshots and all. Also agree with you on the fact that this is different from the other modules out there.
I'm moving this to needs work for one issue, I noticed that your .info says PHP 5.1 is required and your README.txt only says that Drupal 6.x is required. Can you please add PHP 5.1 to the documentation or remove it from the info file if it's not really required? Don't know if this is an application blocker, but hopefully @klausi can undo my mistakes :)
I also found two small things I would like to give my feedback on:
deepsurvey.module line 334 - there are 235 characters in this line and that makes it a little bit hard to read it, but this is my opinion. And it's recommended that you set your branch as default as opposed to having it set to master:
http://drupal.org/node/1659588

Regards,
Caio

In line 74 of the deepsurvey.module the comment seems misleading. When reading // Restrict access if unauthorised. I would then expect return FALSE;, not TRUE. The code is correct. It's the comment that's misleading.

You, also, seem to use double quotes(") instead of single quotes(') for SQL commands. Read about double and single quotes here(check the best answer).
For example, line 113 could become:
$result = db_query('SELECT nid, title FROM {node} WHERE type = "%s"', 'deepsurvey');
Some people say that single quotes are processed faster when no variables exist in the string. The link above also provides links to a comparison between the two types of quotes.

In line 388, it would be better to consider replacing the '<p>survey is closed</p>' with '<p>' . t('survey is closed') .'</p> or even better to set this as a variable and allow the administrator to change it at will.

This is just a suggestion: would't it be better if you changed the function _deepsurvey_previous_item() to something with less arguments; You could use an array or an object variable to group all the $current_* variables in one and still have what you need.

And most important: remove the docs folder. It's 3MB on its own. You could use the advanced_help module to provide the documentation you want.

First off, I have to say that this module has awesome documentation. Great work. Also sounds like a lot of people looking forward to it.

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

A few coding issues as well: http://ventral.org/pareview/httpgitdrupalorgsandboxchrisp1136296git
The references to & are that you shouldn't have references in the function call as it breaks php 5.3: http://php.net/manual/en/language.references.pass.php

Manual Review
- In the menu, I would recommend using % replacements instead of creating menu items for each deepsurvey. I realize it may not be possible as I don't totally understand what you're doing with _deepsurvey_build_menu(). The reason I recommend it if at all possible is to avoid the costly menu_rebuild() function. For example:

    $items['admin/deepsurvey/%deepsurvey_deepsurvey_node'] = array(
      'title' => $line['title'],
      'page callback' => 'deepsurvey_edit_survey',
      'page arguments' => array(2),
      'access arguments' => array('administer deep survey'),
      'type' => MENU_NORMAL_ITEM,
      'file' => 'deepsurvey.admin.inc',
      'child_nodes' => $child_node_array,
    );

- Also recommended to use drupal_write_record instead of INSERT/UPDATE queries. This will allow you to not have to worry about the database type the end user has and will make upgrading to D7 much easier.
- There are 2 queries in deepsurvey_view(). You could get this down to 1 query by pulling the extra field in the first query and then only using it if needed.
- Wrap in t() and capitilization and punctuation: drupal_set_message('problem removing content references from ' . $container_node['type'], 'warning');
- Any theme functions you implement should have your module name in them, preferrably at the beginning to avoid namespace issues: 'add_existing_questions_table'
- You have a couple of super long if statements. I would recommend breaking those down to multiple lines. The place where I noticed it was deepsurvey_question.user.inc:

// IMO, This is more readable.
if ((!$form['code'][$col][$row]
     || ($form['type'][$col][$row]['row']['#value'] == DEEPSURVEY_QUESTION_SINGLE
        && $form['type'][$col][$row]['col']['#value'] == DEEPSURVEY_QUESTION_SINGLE)
     || ($form['type'][$col][$row]['row']['#value'] == DEEPSURVEY_QUESTION_SINGLE
        && $form['type'][$col][$row]['col']['#value'] == DEEPSURVEY_QUESTION_MULTI)
     || ($form['type'][$col][$row]['row']['#value'] == DEEPSURVEY_QUESTION_MULTI
         && $form['type'][$col][$row]['col']['#value'] == DEEPSURVEY_QUESTION_SINGLE))
    && $form['code'][$col][$row]['#type'] != 'select') {

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

I really feel with you, and I am actually impressed that you haven't given up already. There have now been EIGHT people reviewing this single module. It looks like every time you satisfy one reviewer, a new one pops up with a different set of issues.

Hello Cris,

There is still have some coding standered issue, i think it can be easly fix.

link : http://git.drupal.org/sandbox/chris_p/1136296.git

FILE: /var/www/drupal-7-pareview/pareview_temp/deepsurvey.admin.inc
--------------------------------------------------------------------------------
FOUND 6 ERROR(S) AFFECTING 6 LINE(S)
--------------------------------------------------------------------------------
94 | ERROR | unlink() is a function name alias, use domxml_unlink_node()
| | instead
99 | ERROR | Functions must not contain multiple empty lines in a row; found
| | 2 empty lines


FILE: /var/www/drupal-7-pareview/pareview_temp/deepsurvey.install
--------------------------------------------------------------------------------
FOUND 12 ERROR(S) AFFECTING 12 LINE(S)
FILE: ...p/deepsurvey_items/deepsurvey_answer_list/deepsurvey_answer_list.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
FILE: ...w_temp/deepsurvey_items/deepsurvey_question/deepsurvey_question.install
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)

There is still have some coding standered issue, i think it can be easly fix.

I believe the above review may be a particularly good illustration of the temptation to simply run the PAReview script without doing any critical analysis of the code whatsoever. Also, considering the point this discussion is at, the timing is particularly odd.

@srutheesh, if you're interested in helping Chris's Deep Survey project along, I would suggest that you complete a substantive review with clear comments for changes.

Hello,

I guess the way people see PAReview automated tool and coder itself is that once the coding standards are out of the way you can get into the in depth stuff. that's how it seemed with my project application process.

Here's my manual review:

In using the module, I like it. Using the entity API would be awesome for the Drupal 7 version. (This would be so it would have it's own entity infrastructure seperate from content like custom tokens, custom pages, forums and others.)

My first suggestion would be to reduce the amount of the code in the module file by using an includes folder containing include files for different categories as you have done already with the settings form and datetime. This is because lots of unnecessary code will be called each time the module is called, seperating the code into multiple files gives you the opportunity to only call files when needed through the use of module_load_include() and attaching files to the menu and theme systems. Examples would be an include to deal with the deep survey form include and all it's states, another to for dealing with the theme layer etc.

I see you use a lot of database queries and was trying to think of a way you could avoid that as it using the database abstraction layer is a big drain on performance and I thought maybe you could use several helper functions to avoid some of the duplicate query code to help with this although I do not know any other great solution except from the module user using query caching which is obviously nothing to do with the module developer.

Apart from this to me this seems RTBC as there are no major blockers.

And thanks to the review bonus program, this module has now been RTBC for eight week, while the only thing missing is 3 seconds of work (giving you a role of committer) from the right people. >:( This is really becoming my favorite example for how broken the review system is...

@fuzzy76 - complaining doesn't help. AFAIC if you want to earn the review bonus for this application that would be fine ;)

Actually, if I want the process to improve / change for the better, complaining will accomplish more than just submitting to the system. ;) Not by much, but there's not that many other options clear to me. There was a discussion, but nobody seemed to be able to conclude from it, so it went on for a year before just dying.

Complaining almost never helps. Constructive criticism in the right spot (not here) does help.

@fuzzy76: Many reviewers prioritize projects based on how long they've sat idle at a given state ... by posting here, you bump this back up to the top of the list, thus resetting the clock on how long this takes to filter back down to the bottom; and as a result actually increasing the delay for the applicant.

While I haven't looked at the submodules, the parent module shows a good grasp of Drupal APIs; and I wasn't able to find any major issues outstanding. Given the number of other eyes which have looked through it, I'd be pleased to promote it.

Thanks for your contribution, chris_p!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.