# Displaying a map of points

I'm working on an HTML text-based game (think like OGame and other browser games of the genre). I've tried worked on a grid controller to display a map of points that you can move around when you have an entity to move yourself from point to point. The basic concept that I used is to grab a sub-grid of the total map points available, based on a default value hard-coded that should represent the "radius" of the vision you have of your surroundings and pass the points to the view. Each map point (in the screenshot the colored square) corresponds to a specific point in the world where you will eventually build a base and others such actions.

I would like a review primarily at the way I'm constructing the grid. I'm filtering all of the map points of the database even when I only want to show a small portion of the grid.

I'm still at an experimental stage at the moment so I'm looking at all the feedback I can get!

Github link for the whole project.

Controller

class GridController < ApplicationController

before_action :authenticate_user!, only: [:edit, :update, :destroy]
before_action :set_map_point, only: [:point]
before_action :set_map_point_to_move_to, only: [:move]
before_action :redirect_if_no_player, only: [:show]

def show
@player = current_player
@map_point = @player.entity.map_point
@neighbors = @map_point.neighbors
end

def point
render partial: 'map_points/map_point'
end

def move
@player = current_player
@player.entity.move(MapPoint.find(params[:map_point_id]))
redirect_to grid_show_path
end

private

def set_map_point
@map_point = MapPoint.includes(:entities).find(params[:id])
end

def set_map_point_to_move_to
@map_point = MapPoint.find(params[:map_point_id])
end

def redirect_if_no_player
if not user_signed_in?
redirect_to root_path
return
end

if current_user.player.nil?
redirect_to new_player_path
end
end

end


Map Point class

class MapPoint < ActiveRecord::Base
has_many :entities
belongs_to :terrain
validates :x, presence: true
validates :y, presence: true

def neighbors
neighbors = Hash.new
Directions::DIRECTIONS.each{ |direction|
neighbors[direction] = MapPoint.find_by(x: direction.dx + self.x, y: direction.dy + self.y)
}
return neighbors
end

range_map = range / 2
minMaxX = calculateMinMax(range_map, MapPoint.minimum("x"), MapPoint.maximum("x"), self.x)
minMaxY = calculateMinMax(range_map, MapPoint.minimum("y"), MapPoint.maximum("y"), self.y)
MapPoint.includes(:entities).where(:x => minMaxX[0]..minMaxX[1]).where(:y => minMaxY[0]..minMaxY[1]).order(x: :asc, y: :asc).group_by(&:y)
end

private
def calculateMinMax(range_map, minMap, maxMap, position)
overflow = 0
underflow = 0
borderLeft = position - range_map
borderRight = position + range_map
if(borderLeft < minMap)
overflow = borderLeft.abs
end
if(borderRight > maxMap)
underflow = borderRight - maxMap
end
min = [minMap, borderLeft].max
min = min - underflow
max = [maxMap, borderRight].min
max = max + overflow
return [min,max]
end
end


View of the grid

<h1>Grid system</h1>
<div class="row">
<div class="centered">
<%= form_tag do %>
<table class="table-bordered float inline">
<% @map_points.each do |key, array| %>
<tr>
<% array.each do |map_point| %>
<% if map_point.id == @map_point.id %>
<td class="current_zone" bgcolor="<%= map_point.terrain.colour if !map_point.terrain.nil? %>">
<%= radio_button_tag(:id,map_point.id, true, class: "grid_point") %>
</td>
<% else %>
<td bgcolor="<%= map_point.terrain.colour if !map_point.terrain.nil? %>">
<%= radio_button_tag(:id,map_point.id, false, class: "grid_point") %>
</td>
<% end %>
<% end %>
</tr>
<% end %>
</table>
<% end %>
<div class="float inline square border">
<div class="row arrow centered">
<% unless @neighbors[Directions::NORTH].nil? %>
<%= link_to '', grid_move_path(@neighbors[Directions::NORTH]), :class => "btn glyphicon glyphicon-arrow-up centered"  %>
<% end %>
</div>
<div class="row arrow">
<% unless @neighbors[Directions::WEST].nil? %>
<%= link_to '', grid_move_path(@neighbors[Directions::WEST]), :class => "btn glyphicon glyphicon-arrow-left"  %>
<% end %>
<% unless @neighbors[Directions::EAST].nil? %>
<%= link_to '', grid_move_path(@neighbors[Directions::EAST]), :class => "btn glyphicon glyphicon-arrow-right"  %>
<% end %>
</div>
<div class="row arrow centered">
<% unless @neighbors[Directions::SOUTH].nil? %>
<%= link_to '', grid_move_path(@neighbors[Directions::SOUTH]), :class => "btn glyphicon glyphicon-arrow-down centered"  %>
<% end %>
</div>
</div>
</div>
</div>
<div class="row">
<div class="float zone">
<%= render @map_point ||= MapPoint.new %>
</div>
</div>


• @Jonah Github link is up for more code, is that enough for you ?. I don't care about he high-level design if you are talking about the css and style. Dec 1 '15 at 1:15
• The thing is I think having too much code would not benefit the question. I understand what you are saying but I'll wait before adding any other classes, because I don't know what I could add. I do have a dev instance on heroku but it require sign in that we limited the incsription Dec 1 '15 at 1:22
• Added a screenshot for you! Hope it will help! Dec 1 '15 at 1:26
• What do the different colors signify? Also, you said you can move around with an entity -- is this terrain that your character moves through? Dec 1 '15 at 1:44
• The colors represents a type of terrain for the map point, but this is a bit out of scope for the question. I know that I may have bigger problem design but this is out of scope for Code Review I think. I would need to show a bunch of more classes to show the bigger picture. I've tried to add more explanation to help focus on the creation of the grid. Thanks for the interest of the question and sorry if it still not enough. Dec 1 '15 at 2:04

This is just a once-over on the code. I haven't had the chance to really dive in and explore the intention behind the code overall, so this is micro-level refactoring.

Your controller can be reduced to this, I believe:

class GridController < ApplicationController
GRID_SIZE = 10

def show
@point = current_user.map_point
@neighbors = @point.neighbors
@grid = @point.load_grid(GRID_SIZE) # or just grid with the refactored MapPoint class below
end

def point
@map_point = MapPoint.includes(:entities).find(params[:id])
render partial: 'map_points/map_point'
end

def move
current_user.entity.move(MapPoint.find(params[:map_point_id]))
redirect_to grid_show_path
end
end

1. The actions you were filtering through authenticate_user! don't actually exist in your controller. So the filter does nothing and should be removed.
2. However... You always want to authenticate the user. I can't think a reason not to, really. Hence, the before_action filter probably belongs in ApplicationController so it's global - you can always call skip_before_filter in a subclass if you explicitly don't want authentication.
3. If the user doesn't pass authentication, the authenticate_user! methods should automatically perform the redirection (since it's a !-method, I'd expect to redirect or raise an exception). This should not be something you have to implement in different controllers or for specific actions. So that's another filter method removed.
4. set_map_point isn't used.
5. set_map_point_to_move_to is used in one action, and is so specific I see no reason to make it a separate method.
6. There's no reason for doing @player = current_player. Presumably current_player is also available to views as a helper method, so referencing it in an instance variable should be redundant.
7. It'd be nice to add a #map_point method to Player, simply as a shortcut for player.entity.map_point. Stay on the good side of the Law of Demeter.
8. I've changed some variable names to something that fits better (I think). E.g. if you call something load_grid, presumably what you get back is... a grid.
9. I've moved the hard-coded 10 to a constant, so it's not a magic number floating around.

You MapPoint class can no doubt be simplified too. For instance:

class MapPoint < ActiveRecord::Base
has_many :entities
belongs_to :terrain
validates :x, presence: true
validates :y, presence: true

def neighbors
Directions.each.with_object({}) do |neighbors, direction|
neighbors[direction] = MapPoint.find_by(x: direction.dx + x, y: direction.dy + y)
end
end

def grid(range)
x_range = grid_bounds(x, range, MapPoint.minimum(:x), MapPoint.maximum(:x), x)
y_range = grid_bounds(y, range, MapPoint.minimum(:y), MapPoint.maximum(:y), y)
MapPoint.includes(:entities).where(x: x_range, y: y_range).order(x: :asc, y: :asc).group_by(&:y)
end

private

def grid_bounds(position, grid_size, min, max)
half_size = grid_size / 2
min_pos = position - half_size
max_pos = position + half_size
if min_pos < min
(min..[min + grid_size, max].min)
elsif max_pos > max
([max - grid_size, min].min..max)
else
(min_pos..max_pos)
end
end
end

1. Don't use camelCase - Ruby uses snake_case. Certainly don't mix the two. I'm looking at you calculateMinMax!
2. return is implicit at the end of a method. Don't assign stuff to a variable just to return it on the next line; return the expression directly and implicitly.
3. Don't modify an external variable from within a block; use something like each_with_object (case in point: #neighbors). And use do..end for multiline blocks.
4. Directions::DIRECTIONS is clunky. Instead implement each on Directions and include Enumerable. That way you have access to the .with_object modifier, and a host of other neat stuff. (Again, looking at #neighbors). You can also just make it a scope instead of a method.
5. Actually, don't loop at all in #neighbors. Or, rather, don't perform multiple queries in a row. In #load_grid you craft a single query, which is more efficient - betting you can do the same in #neighbors, though I haven't rewritten it to that extent. You can either use ranges to do a WHERE ... BETWEEN ... clause, or list x/y values and do a WHERE ... IN (...) clause.
6. Don't mix hash rockets (=>) and colons when there's no need to.
7. If I understand #calculateMinMax correctly (it's just a clamp-type function, right?), it can be reduce to the 3 lines above. (Edit: Wasn't thinking; it clamps a range, not just a position. I've updated the code to something that's probably more correct.) Also note that it returns a range, since that's what you actually need elsewhere.
8. Look into caching MapPoint.minimum and MapPoint.maximum values if applicable. (Edit: Then again, you could probably just query the database, and see what you get back; if you give a range that extends beyond min or max, you'll just get fewer records... might not be a need to figure out the min/max or to clamp anything.)

I haven't looked at the view (may or may not get the chance to do so), but these are just some rough idea for how to refactor what you have. I'm not saying this'll work (haven't tried it), it's just to show a direction you may want to move in.

Edit: Got a chance to have a look at your view code. Just some brief notes.

1. More an HTML/CSS thing than a Ruby/Rails thing but don't use bgcolor - use a class name instead, and do the styling with css.
2. Consider putting some logic into helper methods or partials. For instance, the directional buttons are pretty straight forward to extract into a partial.
3. This bit worries me:

<%= render @map_point ||= MapPoint.new %>


For one, you could just write render @map_point || MapPoint.new - no need to assign the fallback to anything with =|| (ideally, your views never have to assign anything to anything, least of all to instance vars set by the controller).

But also, why have the fallback? If @map_point is nil, then your controller action would have failed already, since it's calling methods on @map_point. So either you're expecting that it might be nil - but not checking for it elsewhere - or it just can't be nil, and the fallback is unnecessary.

• I don't have the time to look at everything and try every suggestion tonight, but this is really helpful! Some points made me realize that some other parts may need refactoring since you bring it up here! I'm a Java developer at root so I see that it still got reflex (like camelCase). I think I will learn so much! Dec 2 '15 at 1:47
• You don't have to try for the modifications you made if you don't want to! I will play with your suggestions tomorrow and give feedback if you want. Thank you again! Dec 2 '15 at 1:52
• @Marc-Andre Glad you found it useful! And yeah, there's quite a few things - things that'll also impact other code than what you put in the question - so don't rush to do everything. Basically, it just looked to me like you were working too hard; there was just more code than there needed to be. If you find yourself thinking "this should be easier", there probably is an easier way (that's why I like Ruby/Rails). Just try always keeping it high-level/top-down. Dec 2 '15 at 11:17
• @Marc-Andre Just pinging you to say I took a quick look at your view code and updated my answer with a few points Dec 2 '15 at 20:22
• For the nil point I don't remember why I put it there but I know I had a problem with it having nil and solve it that way. Maybe I do not have the problem anymore and since I do not have many tests (shame on me) I have dead code! Thanks again for the edit! Dec 2 '15 at 20:27