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
@map_points = @map_point.load_grid(10)
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_t

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

• The actions you were filtering through authenticate_user! don't actually exist in your controller. So the filter does nothing and should be removed.

• 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.

• 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.

• set_map_point isn't used.

• 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.

• 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.

• 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.

• 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.

• 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  max
      ([max - grid_size, min].min..max)
    else
      (min_pos..max_pos)
    end
  end
end

• Don't use camelCase - Ruby uses snake_case. Certainly don't mix the two. I'm looking at you calculateMinMax!

• 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.

• 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.

• 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.

• 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.

• Don't mix hash rockets (=>) and colons when there's no need to.

• 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.

• 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.

• 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.

• Consider putting some logic into helper methods or partials. For instance, the directional buttons are pretty straight forward to extract into a partial.

-
This bit worries me:

For one, you could just write `render @map_point ||

StackExchange Code Review Q#112390, answer score: 5