jim.bo [jimbo]

1. Your .install file with a table to save 1 tweet seems overkill. Remove it and use the variables functions that Drupal gives you. variable_get, variable_set and variable_del.

2.

  $items['admin/settings/last_tweet'] = array(
    'title' => 'Last Tweet settings',
    'description' => 'Configuration options for Last Tweet module',
    'page callback' => 'last_tweet_admin_settings',
    'access callback' => 'user_access', // TRUE will give access to everyone
    'access arguments' => array('administer site configuration'),
  );

You don't need to fill in access callback, when using the user_access function, it's default.

3.

/**
 * Page callback function for admin/settings/last_tweet.
 */
function last_tweet_admin_settings() {
  return drupal_get_form('last_tweet_settings_form');
}

There is a much easier way to do this. You can do it right in the menu definistion so that you don't need the function above at all.

  $items['admin/settings/last_tweet'] = array(
    'title' => 'Last Tweet settings',
    'description' => 'Configuration options for Last Tweet module',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('last_tweet_settings_form'),
    'access arguments' => array('administer site configuration'),
  );

4. last_tweet_settings_form() has a variable like last_tweet_settings_form(&$form_state)

5. Then you can simplify the last_tweet_settings_form() to.

function last_tweet_settings_form(&$form_state) {  
  $form = array();
  
  $form['last_tweet_username'] = array(
    '#type' => 'textfield',
    '#title' => t('Your Twitter Username'),
    '#default_value' => variable_get('last_tweet_username', ''),
    '#required' => TRUE,
  );
    
  return system_settings_form($form);
}

And you don't need the last_tweet_settings_form_submit(), because system_settings_form() adds another form_submit that saves the "last_tweet_username" value into the variable "last_tweet_username".

6. WRONG WRONG WRONG... Please divide the function into more functions that only do one thing.

/**
 * Implementation of hook_cron()
 */
function last_tweet_cron() {
  get_last_tweet();
}

function get_last_tweet() {
  $sql = "SELECT twitter_username
          FROM {twitter}";
  $result = db_query($sql);
  
  if (db_affected_rows($result) == 1) {
    $row = db_fetch_array($result);
    $username = $row['twitter_username'];
    $url = 'http://api.twitter.com/1/statuses/user_timeline.json?screen_name=' . $row['twitter_username'] . '&count=1';
    $response = drupal_http_request($url);
    
    if (_twitter_request_failure($response)) {
      return array();
    } else {
      $response = json_decode($response->data, true);
      $last_status = $response[0]['text'];
      $created = $response[0]['created_at'];
      $timestamp = strtotime($created);
      $date = date('j M Y', $timestamp);
      
      return $last_status . ' @<a href="http://twitter.com/' . $username . '">' . $username . '</a> on ' . $date;
    }
  } else {
    return false;
  }
}

should be something like this

function last_tweet_get() {
  return variable_get('last_tweet_tweet', '');
}

/**
 * Implementation of hook_cron()
 */
function last_tweet_cron() {
  last_tweet_update();
}

function last_tweet_update() {
	$username = variable_get('last_tweet_username', '');
	if (!empty($username)) {
    $url = 'http://api.twitter.com/1/statuses/user_timeline.json?screen_name=' . $username . '&count=1';
    $response = drupal_http_request($url);
    
    if (_twitter_request_failure($response)) {
      return array();
    } else {
      $response = json_decode($response->data, true);
      $last_status = $response[0]['text'];
      $created = $response[0]['created_at'];
      $timestamp = strtotime($created);
      $date = date('j M Y', $timestamp);
      
      variable_set('last_tweet_tweet', $last_status . ' @<a href="http://twitter.com/' . $username . '">' . $username . '</a> on ' . $date);
    }
  }
}

Some more comments in the code would be nice.

You probably would like to have a theme function that styles and prints out the last tweet.

Hello, and thanks for applying for a CVS account.

See also http://drupal.org/coding-standards to understand how a module should be written; in particular see how Drupal variables, global variables, constants, and functions defined from the module should be named.

Remember to change status when you upload a new version of the code, or reviewers will not know there is something new to review. :-)

Please review :)

1. Functionnames should start with the modules name.

function get_last_tweet()

function update_last_tweet()

function _twitter_request_failure($results) 

should be something like this

function last_tweet_get()

function last_tweet_update()

function _last_tweet_twitter_request_failure($results) 

2. Drupal variable names should be prefixed with the module name; if the module name is last_tweet.module, then the Drupal variable names should be prefixed by last_tweet_.

Wrong:

variable_get('twitter_username', '');

Look over the code again and you will soon have a modules that are ready. The "errors" I have pointed out, can maybe be found at more places in the code.

A suggestion will be to add a readme file, and recommend that cron should be run often, if the twitter is updated often.

See http://drupal.org/coding-standards to understand how a module should be written.

updated some stuff with jim.bo. check it out and let us know.

Think that the & should be remove infront off $form.

  function last_tweet_settings_form_submit(&$form, &$form_state) {

Otherwise the code looks good. Will try it out when I get som free time and report back.

As reported in http://drupal.org/node/751826, the argument $form is not passed by reference.

function test_myform_submit($form, &$form_state) {
  db_query("INSERT INTO {table} (name, log, hidden) VALUES ('%s', %d, '%s')", $form_state['values']['name'], $form_state['values']['access']['log'],  $form_state['values']['hidden']);
  drupal_set_message(t('Your form has been saved.'));
}

Scyther:
Otherwise the code looks good.

Sorry, I missed a few thinks with my eyes. Ran it in Coder, still 4 normal warnings, 1 minor warnings.

Line 81: else statements should begin on a new line

    } else { // OK to proceed

severity: normalLine 82: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE

      $response = json_decode($response->data, true);

severity: normalLine 88: else statements should begin on a new line

  } else {

severity: normalLine 89: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE

    return false;

severity: minorLine 136: Missing period

 * Implementation of hook_cron()

1. Don't forget to change the status to "needs review" when you have added a new revision that should be reviewed.

2. Still 1 normal warnings in Coder.

fixed with jim.bo..

Coder module is pretty cool :)

The code looks fine to me, no warnings from Coder.

Have tested the module and it works.

Approved based on Scyther's RTBC.

Michelle