Spiked [spiked]

      watchdog('DirectAdmin', 'Error: ' . $result['error'] . ' - <em>' . $result['details'] . '</em> <pre>' . print_r($result, true) . '</pre>', array(), WATCHDOG_WARNING);

It should be better to use placeholders, rather than concatenating strings.

; $Id$
name = "Directadmin API"
description = "Allows Drupal contributions to communicate with a DirectAdmin server"
core = 6.x

The module doesn't declare its dependency from PHP 5.

/*
 * Implementation of hook_requirements()
 */
function directadmin_api_requirements($phase) {
  
}

There is no code.

require_once(drupal_get_path('module', 'directadmin_api').'/forms.inc');
require_once(drupal_get_path('module', 'directadmin_api').'/directadmin/directadmin.class.php');

There is another way to include PHP code; if then you are always including those files, it would be better to incorporate them into the module file.

    'title' => 'DirectAdmin data',
    'description' => 'DirectAdmin data.',

The description should be more descriptive.

function directadmin_api_perm() {
  return array('administer directadmin_api');
}

The permission string should be something more descriptive (see administer nodes, administer users, etc...).

/*
 * Custom hooks
 */
function directadmin_api_cmd($cmd, $headers = array(), $method = 'GET', $data = NULL) {
  return DirectAdmin::cmd($cmd, $headers, $method, $data);
}

function directadmin_api_get_free_ips() {
  return DirectAdmin::getFreeIPS();
}

function directadmin_api_get_random_free_ip() {
  return DirectAdmin::getRandomFreeIp();
}

The custom class is used just to define static methods, which are then also exposed through functions; in that case, it should be better to not define such class.

  $form['directadmin_api_reseller_password'] = array(
      '#type' => 'password',
      '#title' => t('Reseller password'),
      '#description' => t('The resellers password, as described above.'),
      '#required' => true,
      '#size' => 30,
      '#default_value' => '', // variable_get('directadmin_api_reseller_password', '')
  );

The code should use the value of the Drupal variable, not an empty string.

function directadmin_api_data_form_validate($form, &$form_items) {
  
}

function directadmin_api_data_form_submit($form, &$form_items)
{
  variable_set('directadmin_api_host', $form_items['values']['directadmin_api_host']);
  variable_set('directadmin_api_port', $form_items['values']['directadmin_api_port']);
  variable_set('directadmin_api_reseller_username', $form_items['values']['directadmin_api_reseller_username']);
  variable_set('directadmin_api_reseller_password', $form_items['values']['directadmin_api_reseller_password']);
  
  drupal_set_message('Settings saved', 'status');
}

An empty validate function is not useful. If the form values are used to set Drupal variables, it should be better to call system_settings_form().

    $tests[] = array(
      'test' => "CMD_API_PACKAGES_USER",
      'result' => "<strong>passed</strong>",
      'info' => "{$count} packages available",
    );

The strings result, and info should be translatable.

  $port = (int) $form_items['directadmin_api_port'];

  if (!is_numeric($port) || $port < 0 || $port > 65535) {
    form_set_error('directadmin_api_port', 'Invalid port number.');
  }

As the value is converted in an integer, it's probable that is_numeric() will return TRUE.

  variable_set('directadmin_api_host', check_plain($form_items['values']['directadmin_api_host']));
  variable_set('directadmin_api_port', check_plain($form_items['values']['directadmin_api_port']));
  variable_set('directadmin_api_reseller_username', check_plain($form_items['values']['directadmin_api_reseller_username']));

check_plain() and similar functions are used when the value is rendered, not when it is saved in a database table.

  drupal_set_message(t('Settings saved'), 'status');

It should be better to use the same string used by Drupal; in that way, the translation people would have a string less to translate.

function directadmin_api_data_test() {}

Why doesn't the code use simpletest.module, for the testing?

      '#default_value' => check_plain(variable_get('directadmin_api_host', '')),

The call to check_plain() is not necessary, as it is already done from the form API; calling it twice creates some problems.

  if ($key === FALSE) {
    return TRUE;
  }
  else {
    return FALSE;
  }

The code can be simplified.

  if (isset($result['valid'])) {
    switch ($result['valid']) {
      case '0':
        return FALSE;
        break;
      case '1':
        return TRUE;
        break;
    }

It could be simply

  if (isset($result['valid'])) {
    return (boolean) $result['valid'];
  }