3
\$\begingroup\$

I've done a bit of modifications and finally got it working, and did a lot of reading about @ vs @@ vs just declaring and I came up with.

It goes to Steam's API to return information (JSON Format) based on the endpoint. A list of games they own, a list of games they recently played, profile information, etc.

class SteamController < ApplicationController

  def initialize
    @steamKey = ENV["steam_api_key"]
    @ISteamUrl = "http://api.steampowered.com/ISteamUser/"
    @IPlayerUrl = "http://api.steampowered.com/IPlayerService/"
  end

  def get_steam_summary_per_user #/steam/:id
    @steamEndpoint = "GetPlayerSummaries/v0002/?key="+@steamKey+"&steamids="
    @steamId = User.find(params[:id]).steamId
    @playerUrl = @ISteamUrl + @steamEndpoint + @steamId.to_s
    @games = HTTParty.get(@playerUrl).to_json
    render json: @games
  end

  def get_steam_recently_played_per_user #/game/:id
    @recPlayedEndpoint = "GetRecentlyPlayedGames/v0001/?key="+@steamKey+"&steamid="
    @steamId = User.find(params[:id]).steamId
    @playerUrl = @IPlayerUrl + @recPlayedEndpoint + @steamId.to_s
    @games = HTTParty.get(@playerUrl).to_json
    render json: @games
  end

  def get_steam_owned_games_per_user #/allGames/:id
    @allGamesUrl = "GetOwnedGames/v0001/?key="+@steamKey+"&steamid="
    @steamId = User.find(params[:id]).steamId
    @playerUrl = @IPlayerUrl + @allGamesUrl +  @steamId.to_s
    @games = HTTParty.get(@playerUrl).to_json
    render json: @games
  end

end

Routes

  get '/steam/:id',  to: 'steam#get_steam_summary_per_user'
  get '/games/:id',  to: 'steam#get_steam_recently_played_per_user'
  get '/allgames/:id', to: 'steam#get_steam_owned_games_per_user'

I originally just has @baseUrl but realized they have multiple services and versions, but even before that I couldn't figure out why doing

@baseUrl = "http://api.steampowered.com/"+@recPlayedEndpoint+"etcetc wouldn't work - I assumed it was related to initializing the `@recPlayedEndpoint and then re-assigning it later.

Ths code feels very redundant to me and I still have 5-6 more functions to build out.

The routes make sense, and I'm not sure if I can do them better. Please also tell me if my syntax is not proper for Rails - While I have an Angular front-end I'm trying to understand when to use camel case vs snake, vs pascal etc etc.

(All of this outputs to pure JSON and I use my Angular side render it, so if there is an easier way to do this. However, this is NOT set up on an api version of Rails)

\$\endgroup\$
  • \$\begingroup\$ Can you please clarify what this code does? \$\endgroup\$ – Phrancis Mar 8 '17 at 2:06
  • \$\begingroup\$ @Phrancis It goes to Steam's API to return information (JSON Format) based on the endpoint. A list of games they own, a list of games they recently played, profile information, etc. \$\endgroup\$ – DNorthrup Mar 8 '17 at 2:28
  • \$\begingroup\$ You shouldn't try to keep state between different calls to your server, there is no guarantee that different servers might not handle different calls or that the server might not be restarted. A more specific suggestion I would make would be to move all the code that interacts with the steam api out into it's own class (probably using the singleton pattern) \$\endgroup\$ – Marc Rohloff Mar 8 '17 at 22:21
2
\$\begingroup\$

You don't need all those @s. Everything in the action methods can be handled with local variables, and the initlization can be made more Rails-like. For instance:

class SteamController < ApplicationController
  BASE_URL = "http://api.steampowered.com".freeze

  before_action :load_user

  def get_steam_summary_per_user #/steam/:id
    endpoint = url_for_path_and_id("/ISteamUser/GetPlayerSummaries/v0002/", @user.steamId)
    render json: HTTParty.get(endpoint).to_json
  end

  def get_steam_recently_played_per_user #/game/:id
    endpoint = url_for_path_and_id("/IPlayerService/GetRecentlyPlayedGames/v0001/", @user.steamId)
    render json: HTTParty.get(endpoint).to_json
  end

  def get_steam_owned_games_per_user #/allGames/:id
    endpoint = url_for_path_and_id("/IPlayerService/GetOwnedGames/v0001/", @user.steamId)
    render json: HTTParty.get(endpoint).to_json
  end

  private

  def url_for_path_and_id(path, id)
    "#{BASE_URL}#{path}?key=#{ENV["steam_api_key"]}&steamid=#{id}"
  end

  def load_user
    @user = User.find(params[:id])
  end
end

Here I'm using Rails's before_action setup to load the user, since that's common for all actions. The constants have been moved to, well, constants, or their values to where they're used. A method has been added to construct the URLs. I'm using the more Ruby-like approach of string interpolation to build the URLs rather than string concatenation.

Yes, I've hard-coded a few strings into the methods, although they do have some parts in common, but I've based it on the sections of a URL: Base (constant), path (varies from method to method, even if parts of it repeat), and query string (appended at the end).

One thing that stands out is that the User model has a steamId attribute: That should be steam_id - in Ruby you should use snake_case for attribute and method names.

However, a different (and, I think, better) approach would be to make a service object to wrap the Steam API. Basically a plain old Ruby class (you can place it in app/services, for instance).

Your controller actions would simply construct a service object, passing in the user, and then call the relevant method to get what they want. E.g. your controller would be just:

class SteamController < ApplicationController
  before_action :create_client

  def get_steam_summary_per_user
    render json: @steam_client.summary
  end

  def get_steam_recently_played_per_user
    render json: @steam_client.recently_played
  end

  def get_steam_owned_games_per_user
    render json: @steam_client.owned
  end

  private

  def create_client
    user = User.find(params[:id])
    @steam_client = SteamAPIClient.new(user)
  end
end

where SteamAPIClient is your service object. Keep the controller skinny, and put the logic elsewhere.

Another thing that stood out is the way you set up your routes. The Rails Way™ would be to make the API part of the User controller. From your application's standpoint, you're trying to get information about a user - the fact that it comes from Steam's API is an implementation detail.

In other words, I'd rather see URLs like:

/users/:id/summary
/users/:id/owned
/users/:id/recent

than URLs like /steam/:id, or /games/:id. If you follow common RESTful API patterns, those URLs indicate that you have a collection of "steams" and a collection of "games", and the :id is used to pick out a particular one. But that's not your model; you don't have multiple "steams". What you have a users, identified by an id, which in turn have some associated data you can fetch.

Or in route-speak:

resources :users do
  member :summary, via: :get
  member :owned, via: :get
  member :recent, via: :get
end

Doing things this way is another reason to extract the actual Steam API-fethcing code into a service object, so you can just call it from the Users controller.

The give-away in your current code is that all your actions are called ..._per_user. Well, if it's per user, then make it per user by making it member actions on the Users controller.

Basic idea is that you treat the Steam API as you would treat the database: It's a data source. You don't put all the database connection and querying logic in a controller, you just put User.find(x), and the rest happens elsewhere. Same here, in a sense.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your detailed response! I felt like I was cluttering up the controller a little bit too much. I mostly wanted to get it working to understand how to properly do it, so your response was very helpful - In reality I'd probably just use the gem/library. I don't think I've ever once made a 'service' for a Rails project so that's very cool to learn. \$\endgroup\$ – DNorthrup Mar 8 '17 at 14:04

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.