E1455, Refactoring the Chart helper #439
Conversation
Added some comments
| ] | ||
| @available_data_types[:line] = [] | ||
| @available_data_types[:pie] = [] | ||
| end |
There was a problem hiding this comment.
The code does not have comments explaining what it does. Please add comments to the code.
| } | ||
| }, | ||
| def self.get_pie_template() | ||
| { |
There was a problem hiding this comment.
Could you place this data in a YAML file?
There was a problem hiding this comment.
Could you clarify on this? As per our understanding, these changes create a basic template with dummy values which then get replaced by the data variable being passed in the initialize call.
I believe the original developers kept these values there to show future developers how the values should be for the chart to work.
I was under the impression that yaml files are used for doing environment specific configuration things.
There was a problem hiding this comment.
Generally, it is preferred to segregate different formats (i.e. ruby, html, javascript, etc.). Raw data is no exception. The configuration files in Rails such as database.yml are used directly in Ruby to access the associated information without cluttering the ruby code (Expertiza::Application.config.database_configuration[Rails.env], for example). You can load an individual file like so: YAML.load_file('config/database.yml'). It may be wise to create a config/charts directory which contains files such as bar.yml, pie.yml, etc.
Your second concern about readability is a good one. YAML is, in fact, designed to be human-readable, so all these large data objects will be much easier to understand than if defined in Ruby.
|
Please merge the current rails4 branch. |
…rails4 Merging to get the latest changes in main repository
|
Changes Unknown when pulling fb22796 on noelmathew:rails4 into * on expertiza:rails4*. |
What it does: