[D6] TCMB

Hi, thank you for your contribution.

Initial screening cleared:
-Link to sandbox
-Module duplication
-README.txt file
-No LICENSE files

Please run your module through the Coder module on minor(most). Don't add CVS tags if it tells you to, though.

Functions should all be documented.

Functions should be separated by a black line.

Remove package = TCMB. Packages are only used for established packages or if your module is a package itself.

Also, I would recommend you update your README.txt file to be a little bit more descriptive in the installation and setup of the module.

The latter notes were just an extra favor, so I won't set the status to needs work. Just fix the issues and push the changes to your sandbox please!

Please cleanup before asking for a review. Commented code that is still referenced (see schema) should not be there.

- It's not necessary to check for tables' existence during hook_install(). Just call drupal_install_schema() directly.
- Module description should start with a third person singular verb: "Shows exchange rates...."
- Your installation instructions are incomplete. The block cannot be enabled before the module itself has been enabled.
- Do not use t() for schema descriptions.
- Remove commented code.
- You define the permission "administer tcmb", but you never use it.
- Form element titles should describe the input for the element, not what the user has to do. You should use "Currency code" instead of "Input the currency code for...".
- Make a dedicated function that returns currency codes. You have two arrays that list all currency codes. A dedicated function lowers the chance of errors and makes maintenance easier. If you use keys for codes and values for human-readable currency names (and display those in the interface), you can improve usability.
- Always put comments on a separate line.
- In tcmb_settings_form_submit() you use '%d' as a placeholder. Only %s should be quoted. %d shouldn't.
- Use system_settings_form() for your settings form.
- DON'T USE INTERFACE TEXTS IN ALL CAPS. IT'S BAD FOR READABILITY, MMKAY?
- Instead of using a rather complicated form setup, convert the currency select element to checkboxes. With system_settings_form(), you only need a form builder and no validate or submit handlers.

//***** = reviewer comment

   2 // $Id$

//*****  = This is no longer used.

   3 
   4 /**
   5  * @file
   6  * Main module file for TCMB module.
   7  */

//*****  = This should explain what this module does in brief.

   8 
   9 /**
  10  * Implementation of hook_block().
  11  */
  12 
  13 function tcmb_block($op = 'list', $delta = 0, $edit = array()) {
  14   switch ($op) {
  15     case 'list':
  16     $blocks[0]['info'] = t('TCMB');
  17     return $blocks;

//*****  = Wrong indenting and too less info for the blocks page.

  18     case 'view':
  19     $blocks['subject'] = t('TCMB rates');
  20     $blocks['content'] = tcmb_get_curcode();
  21     return $blocks;

//*****  = Wrong indenting.

  22   }
  23 }
  24 
  25 /*
  26  * Array for currency codes
  27  */

//*****  = Missing @return for what this returns in doc.

  28 
  29 function _tcmb_cur_codes() {
  30   $tcmb_cur_codes = array (
  31     'USD' => t('American Dollar'),
  32     'CAD' => t('Canadian Dollar'),
  33     'XDR' => t('Special D. Rights'),
  34     'DKK' => t('Danish Krone'),
  35     'SEK' => t('Swedish Krona'),
  36     'CHF' => t('Swiss Franc'),
  37     'NOK' => t('Norwegian Krone'),
  38     'JPY' => t('Japanese Yen'),
  39     'SAR' => t('Saudi Arabian Riyal'),
  40     'KWD' => t('Kuwaiti Dinar'),
  41     'AUD' => t('Australian Dollar'),
  42     'EUR' => t('Euro'),
  43     'GBP' => t('Great Britain Pounds'),
  44     'RUB' => t('Russian Ruble'),
  45     'RON' => t('Romanian Leu'),
  46     'IRR' => t('Iranian Riyals'),
  47     'BGN' => t('Bulgarian Lev'),
  48     'DEM' => t('German Marc'),
  49     'BEF' => t('Belgian Franc'),
  50     'LUF' => t('Luxembourgian Franc'),
  51     'ESP' => t('Spanish Peseta'),
  52     'FRF' => t('French Franc'),
  53     'IEP' => t('Irish Pound'),
  54     'ITL' => t('Italian Lira'),
  55     'NLG' => t('Dutch Guilder'),
  56     'ATS' => t('Austrian Schilling'),
  57     'PTE' => t('Portuguese Escudo'),
  58     'FIM' => t('Finnish Mark'),
  59     'GRD' => t('Greek Drachma'),
  60   );
  61 }

//*****  = This is nice, but unfortunately never used again.

  62 
  63 /*
  64  * Gets the currency rates from remote XML file
  65  * and display them in a table
  66  */

//*****  = @return is missing.

  67 
  68 function tcmb_get_curcode() {
  69   $tcmbXML = new DomDocument();
  70   $tcmbXML->load('http://www.tcmb.gov.tr/kurlar/today.xml');
  71   $tcmbXPath = new DOMXPath($tcmbXML);
  72   $header = array('', t('BUYING'), t('SELLING'));
  73   $currency = array();
  74   $rows = array();
  75   $curcodes = variable_get('tcmb_currency', $currency);
  76   if (!$curcodes) {
  77     dsm(t('You need to specify some currency codes in TCMB settings page.'));

//*****  = dsm is not available for users not having devel module. You want to use watchdog here as the end user should not see the error.

  78     return;

//*****  = I think this should return FALSE.

  79     }
  80   foreach ($curcodes as $tcmbcurcode) {
  81     if (!$tcmbcurcode) {
  82       continue;
  83     }
  84     $query = "//Currency[@CurrencyCode='$tcmbcurcode']";
  85     $results = $tcmbXPath->query($query);
  86     $curcode = $tcmb_cur_codes[$tcmbcurcode];
  87     $buy = $results->item(0)->getElementsByTagName('ForexBuying')->item(0)->nodeValue;
  88     $sell = $results->item(0)->getElementsByTagName('ForexSelling')->item(0)->nodeValue;
  89     $rows[] = array($tcmbcurcode, $buy, $sell);
  90     }
  91     $output = theme('table', $header, $rows);
  92     return $output;
  93 }
  94 
  95 /**
  96  * Implementation of hook_menu().
  97  */
  98 
  99 function tcmb_menu() {
 100   $items = array();
 101   $items['admin/settings/tcmb-settings'] = array(
 102     'title' => 'TCMB settings',
 103     'description' => 'Configure TCMB settings',
 104     'page callback' => 'tcmb_admin_settings',
 105     'page arguments' => array('tcmb_admin_settings'),

//*****  = This is wrong, the helper function is not needed. Use page_callback: drupal_get_form and page_arguments: tcmb_settings_form

 106     'access arguments' => array('administer site configuration'),
 107   );
 108   return $items;
 109 }
 110 
 111 /*
 112  * Calls the admin settings form.
 113  */
 114 
 115 function tcmb_admin_settings() {
 116   return drupal_get_form('tcmb_settings_form');
 117 }

//*****  = Not necessary.

 118 
 119 /*
 120  * Generates the form and allows the admin to set the currency code(s).
 121  */
 122 
 123 function tcmb_settings_form() {
 124   $form = array();
 125   $form['tcmb_currency'] = array(
 126     '#title' => t('Currency code'),
 127     '#default_value' => variable_get('tcmb_currency', array()),
 128     '#type' => 'checkboxes',
 129     '#options' => array (

//*****  = Use your helper function here.

 130     'USD' => t('American Dollar'),
 131     'CAD' => t('Canadian Dollar'),
 132     'XDR' => t('Special D. Rights'),
 133     'DKK' => t('Danish Krone'),
 134     'SEK' => t('Swedish Krona'),
 135     'CHF' => t('Swiss Franc'),
 136     'NOK' => t('Norwegian Krone'),
 137     'JPY' => t('Japanese Yen'),
 138     'SAR' => t('Saudi Arabian Riyal'),
 139     'KWD' => t('Kuwaiti Dinar'),
 140     'AUD' => t('Australian Dollar'),
 141     'EUR' => t('Euro'),
 142     'GBP' => t('Great Britain Pounds'),
 143     'RUB' => t('Russian Ruble'),
 144     'RON' => t('Romanian Leu'),
 145     'IRR' => t('Iranian Riyals'),
 146     'BGN' => t('Bulgarian Lev'),
 147     'DEM' => t('German Marc'),
 148     'BEF' => t('Belgian Franc'),
 149     'LUF' => t('Luxembourgian Franc'),
 150     'ESP' => t('Spanish Peseta'),
 151     'FRF' => t('French Franc'),
 152     'IEP' => t('Irish Pound'),
 153     'ITL' => t('Italian Lira'),
 154     'NLG' => t('Dutch Guilder'),
 155     'ATS' => t('Austrian Schilling'),
 156     'PTE' => t('Portuguese Escudo'),
 157     'FIM' => t('Finnish Mark'),
 158     'GRD' => t('Greek Drachma'),
 159     ),
 160 );
 161     return system_settings_form($form);
 162 }

And thats all I could find for now ...

Best Wishes,

Fabian

Please also add Caching via cache_set / cache_get as spoken in IRC.

You can add a timestamp, too to re-run this every 24h or something configurable.

Querying a remote server on each page_request is a performance no-go and will lead to agry issues against your module like

"I installed tmcb, because I really liked its functionality, but now my site is sooo slow. Pages take ages to load. I disabled the module and it was fast again ..."

Best Wishes,

Fabian

You *need* to cache your data. Otherwise your module will execute a HTTP request every. single. page load, which is a VERY BAD idea.

Also, what happens when the HTTP request fails or the received data is invalid? Will your module crash like a 747 on a freshly mowed lawn (it ain't pretty) or will it gracefully degrade/fail, without visitors noticing it?

Hi Kartagis,

Unfortunately this still needs more work.

Caching is ineffective as of now.

It needs to be done like:

function tcmb_get_curcode() {

  $header = array('', t('Buying'), t('Selling'));
  $rows = tcmb_get_cached_curcode();
  $output = theme('table', $header, $rows);

  return $output;
}

function tcmb_get_cached_curcode($reset = FALSE) {
  $cache = cache_get("tcmb_output");

  $rows = array();

  if (!reset && ($cache && isset($cache->data) && $cache->expire < time())) {
    $rows = $cache->data;
  }
  else {
     $rows = tcmb_retrieve_rows();
     cache_set("tcmb_output", $rows, "cache", 60*60); // Cache for one hour
  }

  return $rows;
}

where tcmb_retrieve_rows does the XML stuff.

Also you did not do all of my suggestions, yet. Please review them again :-).

Thanks and Best Wishes,

Fabian

PS: Edited to seperate theming and data retrieval.

As spoken in IRC: First read and understand all links and do everything that was asked from you here.

Make a checklist of things that were asked here and explain how you solved them all.

Just then come back.

You are wasting everyone's time (here and in #drupal*) by refusing to learn basic PHP^*.

This, in turn, leads to the current situation where #drupal* is writing a module by providing input to the proverbial infinite monkeys. It may get there in the end, but efficient it is not.

Steps to take in order:

1. Learn PHP (good book, courses)
2. Play around with Drupal APIs
3. Try again

*) I realize you've said you do want to learn PHP, but only after this module is finished. That's the wrong way around.

We tried that for weeks and yet you did not learn. You asked us to review your code that turned out to be pretty much identical to our example code. We pointed you to documentation, which apparently you did not read.

Bottom line: even if this module passes the checks, there is a consensus you are unable to maintain it, as you have told us you cannot program and are not willing to learn.

Based on the discussion here, I suggest that this module exist as a sandbox for a bit longer so it can see more real world usage. There are no bugs in the queue, so it appears nobody (other than the author) is actually using this code. Real world usage will help to iron out any performance bugs and also give a chance for Kartagis to experience Drupal community standards a bit more.

It also appears there are some bugs in the module #1249070: Filter data from tcmb.gov.tr prior to printing it and #1249074: Unused variables.

One example of standards where you could benefit is in commit messages and use of the issue queue. I see a lot of commits, but no issues in the queue. I suggest you create issues for work you do prior to doing it and post patches there. Then in your commits, rather than something that conveys little information like "Fixed a function, and removed another function accordingly" (from this commit) you can follow the standard for commit messages.

To raise awareness of the module I suggest you write a longer project page. See this advice for module owners to get a sense of some things you can do.

subscribe. Hoping you can work your way out of this. You've got some pretty important contributors feeling frustrated with you. greggles gave you some good guidance.

Seems to me like @Kartagis has been pretty responsive here. @greggles are you willing to let it go forward at this point? I think it might be stalled on your reservations.

It sounds like this module does the same thing as http://drupal.org/project/currency

The Currency module gets its data from Yahoo Finance, but also provides a way to easily add other data providers. The Currency module has actively solicited patches that add other data providers. It seems to me that it would be better to make TCMB a data provider for the Currency module rather than start from scratch and partially re-implement Currency without all of Currency's features*.

*For instance, caching, which is raised as an issue above, has been a part of Currency for a very long time.

@Kartagis has been responsive throughout the cycle here... For the purposes of approving Kartagis for full project privileges, I'd like to stay on track and not offer redundancy objections at this late date. I do understand your point, @TR. Just seems unfair :-)

@rfay: I understand that it may seem unfair at this late date. However, applicants are explicitly asked to consider and address the duplication issue as part of their application, so this is something that should have been raised by the applicant early on. I think it would benefit the community if the existing Currency module were enhanced, and would allow the applicant to get the benefit of all the other features Currency provides while still being able to use TCMB as a data source. Because of the history of this issue, I didn't change the status of the application and I'm not recommending the application be denied. I think this is a reasonable contribution, but I think it would be much more useful if it were part of the Currency module (3000+ users, D6 and D7 versions available) rather than only serving a few users who need the TCMB data source.

From http://drupal.org/node/1187664:

Search for similar modules, and explain how your application is different.
This demonstrates that you have taken the time to look for existing solutions, and an awareness of Drupal's 'collaboration over competition' ethos.
If you do find existing modules which are similar or related to the functionality of your project (even if only in name), expect reviewers to suggest instead providing your module as a feature or patch to the existing project. Be sure you've read the information at this link, and include your reasoning for the 'new module' approach in your application.

OK, thanks @TR. It sounds like you're making a reasonable recommendation to @Kartagis for the future, and not blocking this approval process. @Kartagis's responses to this may also have helped.

@rfay, definitely. It's been roughly a month since the last discussions and I do appreciate the effort @Kartagis has put into following Drupal standards and best practices in that time.

Thanks for your contribution, Kartagis! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

Upper case -> lower case fixed.

@Kartagis just went through this with another project. It requires Sam's intervention to change the shortname in the repository, sadly.

You can, however, create a new project, move the issues that matter to it, and copy the content of the project page, and push the repo to it (git remote set-url origin you@git.drupal.org:project/newname.git; git push --all)

Otherwise it will take some time to get the repository name changed.