HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyrailsMinor

Parsing GPS data

Submitted by: @import:stackexchange-codereview··
0
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_

Solution

On the spectrum of what-a-class-is-responsible-for, the 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.