patternrubyrailsMinor
Parsing GPS data
Viewed 0 times
gpsparsingdata
Problem
Recently I sent this code snippet as CV attachment and I got simple answer - this code smells and we employ another developer.
I'm new to RoR, so guys can you give me directions to refactor this code and make it amazing?
This code is for parsing uploaded file with GPS data in 3 supported formats CSV, xml, tes (binary). And "answering to controller and other model calls".
```
class Track :destroy
has_many :points, :through => :tracksegments
before_save :parse_file
def calc_results(range_from, range_to)
fl_time = 0
distance = 0
speed = 0
is_first = true
prev_point = nil
track_points = get_track_data
track_points.each do |current_point|
is_last = true
if current_point[:elevation] = range_to
is_last = false
if is_first
is_first = false
if current_point[:elevation] != range_from && prev_point.present?
elev_diff = range_from - current_point[:elevation]
k = elev_diff / current_point[:elevation_diff]
fl_time = (current_point[:fl_time] * k).round(1)
distance = (current_point[:distance] * k).round(0)
end
next
end
fl_time += current_point[:fl_time].round(1)
distance += current_point[:distance].round(0)
end
if is_last && fl_time > 0
if current_point[:elevation] fl_time,
:distance => distance,
:speed => speed}
end
def get_charts_data
get_track_data.to_json.html_safe
end
def get_earth_data
data = get_track_data.map { |x| {:latitude => x[:latitude],
:longitude => x[:longitude],
:h_speed => x[:h_speed],
:elevation => x[:abs_altitude].nil? ? x[:elevation] : x[:abs_altitude]} }
data.to_json.html_safe
end
def get_max_height
get_track_data.max_by{ |x| x[:elevation] }[:elevation].round
end
def get_min_height
get_track_
I'm new to RoR, so guys can you give me directions to refactor this code and make it amazing?
This code is for parsing uploaded file with GPS data in 3 supported formats CSV, xml, tes (binary). And "answering to controller and other model calls".
```
class Track :destroy
has_many :points, :through => :tracksegments
before_save :parse_file
def calc_results(range_from, range_to)
fl_time = 0
distance = 0
speed = 0
is_first = true
prev_point = nil
track_points = get_track_data
track_points.each do |current_point|
is_last = true
if current_point[:elevation] = range_to
is_last = false
if is_first
is_first = false
if current_point[:elevation] != range_from && prev_point.present?
elev_diff = range_from - current_point[:elevation]
k = elev_diff / current_point[:elevation_diff]
fl_time = (current_point[:fl_time] * k).round(1)
distance = (current_point[:distance] * k).round(0)
end
next
end
fl_time += current_point[:fl_time].round(1)
distance += current_point[:distance].round(0)
end
if is_last && fl_time > 0
if current_point[:elevation] fl_time,
:distance => distance,
:speed => speed}
end
def get_charts_data
get_track_data.to_json.html_safe
end
def get_earth_data
data = get_track_data.map { |x| {:latitude => x[:latitude],
:longitude => x[:longitude],
:h_speed => x[:h_speed],
:elevation => x[:abs_altitude].nil? ? x[:elevation] : x[:abs_altitude]} }
data.to_json.html_safe
end
def get_max_height
get_track_data.max_by{ |x| x[:elevation] }[:elevation].round
end
def get_min_height
get_track_
Solution
On the spectrum of what-a-class-is-responsible-for, the
For example, the CSV, XML, TES parsers could each live in their own class, and each of those classes could inherit from a track_parser class. If there's a bug in CSV parsing, having XML parsing code in the file is likely to be noise rather than useful...particularly as an application grows larger. Likewise, if parsing in general needs to change, then a
A few small items that jump out:
-
Magic Number: 3.6 on lines 66, 340 and 341.
-
Like any mutable operation, calling
-
Use of
-
Track sits closer to the Swiss Army knife end than the grapefruit fork end. Though both are legitimate approaches, Swiss Army knives typically require justification. Otherwise, some people consider refactoring appropriate.For example, the CSV, XML, TES parsers could each live in their own class, and each of those classes could inherit from a track_parser class. If there's a bug in CSV parsing, having XML parsing code in the file is likely to be noise rather than useful...particularly as an application grows larger. Likewise, if parsing in general needs to change, then a
track_parserclass provides a place to document/implement/enforce the change. A few small items that jump out:
-
Magic Number: 3.6 on lines 66, 340 and 341.
-
Like any mutable operation, calling
reverse! on a data structure is not considered functional style programming and Ruby programmers tend toward a functional programming style. Calling it twice on the same array [Lines 137/139 and 366/370] suggests walking the array with a decreasing index as an alternative.-
Use of
if rather than case...when... in parse_file is not idomatic Ruby.-
track_points appears 46 times in the code and has structure, should it be a class?Context
StackExchange Code Review Q#67538, answer score: 2
Revisions (0)
No revisions yet.