5
\$\begingroup\$

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_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

  def load_grid(range)
    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>

picture of the grid

\$\endgroup\$
5
  • \$\begingroup\$ @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. \$\endgroup\$
    – Marc-Andre
    Dec 1 '15 at 1:15
  • \$\begingroup\$ 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 \$\endgroup\$
    – Marc-Andre
    Dec 1 '15 at 1:22
  • \$\begingroup\$ Added a screenshot for you! Hope it will help! \$\endgroup\$
    – Marc-Andre
    Dec 1 '15 at 1:26
  • \$\begingroup\$ What do the different colors signify? Also, you said you can move around with an entity -- is this terrain that your character moves through? \$\endgroup\$
    – Jonah
    Dec 1 '15 at 1:44
  • \$\begingroup\$ 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. \$\endgroup\$
    – Marc-Andre
    Dec 1 '15 at 2:04
5
\$\begingroup\$

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.

\$\endgroup\$
6
  • \$\begingroup\$ 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! \$\endgroup\$
    – Marc-Andre
    Dec 2 '15 at 1:47
  • \$\begingroup\$ 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! \$\endgroup\$
    – Marc-Andre
    Dec 2 '15 at 1:52
  • 1
    \$\begingroup\$ @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. \$\endgroup\$
    – Flambino
    Dec 2 '15 at 11:17
  • 1
    \$\begingroup\$ @Marc-Andre Just pinging you to say I took a quick look at your view code and updated my answer with a few points \$\endgroup\$
    – Flambino
    Dec 2 '15 at 20:22
  • \$\begingroup\$ 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! \$\endgroup\$
    – Marc-Andre
    Dec 2 '15 at 20:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.