Follow Drupal Coding Standards

There are other elseif expressions that don't meet Drupal's coding standards, I suggest also fixing them.

Hi @nikhilsukul, I noticed you have made several changes and they all look good to me.
I think there is room for further improving the comments which are not well written and don't end with a period. If you're willing to change them I can give suggestions and you add them into code.
Also, do you agree we should close the #2580193: Wrong standard for writing conditional statement (ElseIf)? If you will close it, please mark it as duplicate of this issue.
Could we have some input from a maintainer before making further change.
So far, this is RTBC.

Thanks ikhilsukul, will make the suggestions in the next two days.

We need to correct the docblocks, improve the comments and remove the white-spaces. Here are some suggestions:

Line 47-56
/**
* Determines if node should be hidden and user
* has permission to have the title hidden.
* @param $node
* Can be a node object or integer value (nid)
* @param $view_mode
* View mode, e.g. 'full', 'teaser'...
* @return
* Returns boolean TRUE if should be hidden, FALSE when not
*/

I believe should be:
/**
* Determine whether a user has privilege and whether to exclude the node title.
*
* @param object $node
* The node object.
* @param string $view_mode
* The view mode, e.g. 'full' or 'teaser'.
*
* @return bool
* Returns TRUE if title should be hidden, otherwise returns FALSE.
*/

Line 82
Remove the inline comment. I think it is not needed.

Line 110
// Also remove the field from the form

Suggest:
// Prevent access to the title field on nodeform view mode.

Line 176
// make sure user have permissions correct

Suggest:
// Exclude the node title for privileged users.

Line 175-176
// --------------
// make sure user have permissions correct

Suggest:
// Prevent users without permission from accessing the node title settings.

Line 181-192
// don't bother to add form element if the content type isn't configured
// to be excluded by user...

Suggest:
// Add form element only for enabled content types.

Line 193
// get tnid

Suggest:
// Get the translation source node ID.

Line 229
// when deleting a content type, we make sure and clean our variable :)

Suggest:
// Remove variables when a content type is deleted.

Line 236-242
/**
* Tells if node should get hidden or not.
* @param $param
* Can be a node object or integer value (nid)
* @return
* Returns boolean TRUE if should be hidden, FALSE when not
*/

I believe should be:
**
* Determine whether the node title should be excluded.
*
* @param object $param
* Contains the node object.
* @param string $view_mode
* The view mode, e.g. 'full' or 'teaser'.
*
* @return bool
* Returns TRUE if the title should be hidden, otherwise returns FALSE.
*/

Line 271
// we look for the nid list

Suggest:
// Prepare a list of the node IDs to be excluded.

Line 276
// get tnid

Suggest:
// Get the translation source node ID.

Line 307-313
/**
* Helper function to _exclude_node_title() that extracts node information from $param.
* @param $param
* Can be a node object or integer value (nid)
* @return
* Returns an array with node id and node type, or FALSE if there were errors.
*/

I'll leave this one for you; please refer to the docblock comment standards.

Line 315
// we accept only integer and object

Suggest:
// Do not load nodes unless we have objects and integers.

Line 320
// if numeric, load the node with nid

Suggest:
// Load the node by its ID if we have numeric value.

Line 329
Remove the inline comment.

Line 332
// Check that the node exists

Suggest:
// Do not get node IDs for non-existing nodes.

Line 338
Remove the inline comment.

After I did some progress on this, I found out the $node and $param may not always be objects but can contain the nid. Please verify that and make necessary changes.

Also, remember to remove the white-spaces - there are several.

Thanks,
Yousif

@nikhilsukul, we should check the other files in the module.

exclude_node_title.info
Change the description to:
Allows exclusion of the title in preselected content type. Works with teaser and other view modes.
package = Access

I believe these line should be removed:
files[] = exclude_node_title.info
files[] = exclude_node_title.module
files[] = exclude_node_title.admin.inc

exclude_node_title.install
// attach default permission to use exclude node title
-->
// Insert in the database the permission to use exclude node title.

$res
-->
$role_ids

exclude_node_title.admin.inc
Add a period to end of the first two comments including the one in @file.

Add a comma to the end of '#disabled' => !module_exists('search')

Select if you wish to remove title from search pages. You need to have Search module !url.
-->
Enable title exclusion in search pages. The Search module must be installed: !url.

If you hide title from one node, it will automatically hide title for all the translated versions of that node. You need to have Content translation module !url.
-->
Enable title exclusion in translated versions of nodes. The Content Translation module must be installed: !url.

Add a comma to the end of '#disabled' => !module_exists('translation')

Exclude title by content-types
-->
Exclude the title by content type

Define title excluding settings for each content type.
-->
Select the content types for which the node title will be excluded.

Remove the t() from '#title' => t($title), as only literals should be passed to the t() function if possible.

Indentation is incorrect on line 75.

nikhilsukul excellent effort! Can we make further changes.

1. +++ b/exclude_node_title.admin.inc
   @@ -1,33 +1,33 @@
   +    '#description' => t('Enable title exclusion in search pages. The Search module must be installed: !url.', array('!url' => l(t('enabled'), 'admin/modules/list'))),
   

   '#description' => t('Enable title exclusion in search pages. The Search module must be !enabled.', array('!enabled' => l(t('enabled'), 'admin/modules/list'))),

2. +++ b/exclude_node_title.admin.inc
   @@ -1,33 +1,33 @@
   +    '#description' => t('Enable title exclusion in translated versions of nodes. The Content Translation module must be installed: !url.', array('!url' => l(t('enabled'), 'admin/modules/list'))),
   

   '#description' => t('Enable title exclusion in translated versions of nodes. The Content Translation module must be !enabled.', array('!enabled' => l(t('enabled'), 'admin/modules/list'))),

3. +++ b/exclude_node_title.module
   @@ -4,12 +4,12 @@
   - * This is the main module file for Exclude Node Title. 
   
   @@ -144,7 +146,7 @@ function exclude_node_title_node_insert($node) {
   +  if (isset($node->exclude_node_title) && $node->exclude_node_title == 1) {
   

   Please check if this works:
   if (isset($node->exclude_node_title) && $node->exclude_node_title == TRUE) {

4. +++ b/exclude_node_title.module
   @@ -166,31 +170,28 @@ function exclude_node_title_check_perm($node) {
    function exclude_node_title_form_alter(&$form, &$form_state, $form_id) {
   

   This function is missing a return statement at the end. For consistency I think we should return FALSE since it doesn't always validate.

5. return ds_render_field($field);
   

   This looks like a display suite function. Do we need a dependency to ds in the .info file. What do you think?

I also noticed there is a duplicate #options attribute in exclude_node_title.admin.inc
After you remove the duplicate line, you may need to put each array element in one line to avoid the 80 character limit.

Great work!

+++ b/exclude_node_title.info
@@ -1,8 +1,6 @@
-package = Other
+dependencies[] = ds
+package = Access

What is the Access package?

Why does ENT depend on DS?

That function is only called if our hook_ds_fields_info_alter() is called, which happens if Display Suite is already installed.

Committed, thanks!

Hi Nikhil,

Things have been so hectic this last two weeks I haven’t taken the time I should have to review the patch and to thank you for the excellent work you have done.

Your work is very much appreciated…and for sure will make a difference to this module.

Thanks again.

Yousif