Zombie Smack Down game

So about a year and a half ago, me and my friend started on a game called Zombie Smack Down. It's a text based game written in Ruby. But we were both really new to Ruby when most of it was written. I realize lots of this code is probably really bad, but I'm kinda stuck in my ways.

I'd love to know which parts of it can be refactored, and changed to follow Ruby best practices, in the game and in the tests The code is also on Github here, but that'll probably get updated and no longer be the same as the code on this site. (and if you find any camel case methods, or variables please let me know, but I tried to remove all that, but I may have missed some. I know it's bad practice in Ruby, I was younger when I made much of this.) If you install if from the one-liner on github, you don't have to enter your correct password, but if you do it'll add a really outdate manual page. Sorry, the installer and updater don't work on Windows and probably not Linux either. I don't have a way to test that.

And I know a lot of this can be updated for Ruby 2 syntax, but I need it to be compatible with the computers at my school, which have 1.8.7 installed, and jailbroken iPods, using Ruby 1.8.6 which is why there a lot of parenthesis around things like puts, because it gives warnings. For now, the tests only really need to work in Ruby 2.1.1, I'm not even sure how well it works in 1.8.7/1.8.6

One last thing, the signal trapping is not working, I tried typing Ctrl-c and it didn't quit. I'd love help with that, but I'm asking this question for a "Code Review". It's just a side note.

requie/player.yml, scores.yml, and tests.rb are on Github only because it said I had too many charecters in my question.

zsd:

```
#!/usr/bin/env ruby

$rpath = "#{File.expand_path '~'}/.zsd"

require "optparse"
$options = []

OptionParser.new { |opts|

opts.on("-i", "--info", "Shows info on current game") {
$options ").split
command = input[0]

if cli.commands.include

I would like to start by saying that I've never worked with ruby 1.8.7, so I apologize in advance if some of my observations are not relevant for that version...

Require relative

You use $rpath where you assume that your app is installed somewhere, which is a very big assumption. Don't do that.

I know, require_relative is not supported on 1.8.7, but it appears to have a workaround. This way you can install your app anywhere, and it will still work.

Global methods

You use global methods to drive your printing. Global methods are 'magical', in the sense that a reader cannot be sure where they came from.

A better solution might be to put these methods in a module, maybe call it ColoredLogger, and include it in classes you want to use it in.

Patching classes

You are patching the String class. Patching a class should be done very rarely, as it may affect unaware third parties.

Your use of patching seems hardly warranted, as you add for an internal single use in the same place you define it.

Simply define the color table anywhere else:

Colors = {
black: { prefix: 30, suffix: 0 },
red: { prefix: 31, suffix: 0 },
green: { prefix: 32, suffix: 0 },
brown: { prefix: 33, suffix: 0 },
blue: { prefix: 34, suffix: 0 },
magenta: { prefix: 35, suffix: 0 },
cyan: { prefix: 36, suffix: 0 },
gray: { prefix: 37, suffix: 0 },
bg_black: { prefix: 40, suffix: 0 },
bg_red: { prefix: 41, suffix: 0 },
bg_green: { prefix: 42, suffix: 0 },
bg_brown: { prefix: 43, suffix: 0 },
bg_blue: { prefix: 44, suffix: 0 },
bg_magenta: { prefix: 45, suffix: 0 },
bg_cyan: { prefix: 46, suffix: 0 },
bg_gray: { prefix: 47, suffix: 0 },
bold: { prefix: 1, suffix: 22 },
reverse_color: { prefix: 7, suffix: 27 }
}

def colorize(color, string)
  "\033[#{Colors[color][:prefix]}m#{string}\033[#{Colors[color][:suffix]}m
end

or even simply do it hard coded in your methods.

Your Array patch seems much more relevant than the String patch.

Variables vs. constants

Your CLI class's initializer populates combos and commands with hard coded data which never changes. They should be declared as constants:

class CLI
  COMMANDS = %w[ kick punch combo combolist taunt info scores quit help heal easter ]
  COMBOS = { "kick punch" => KickPunch.new,
    # ..
  }

What are classes for?

Your Combo classes and Zombie classes are not really classes, but configuration - they do not add any functionality, and therefore should not be classes.

You can use JSON configuration files, where you can keep the names and vitals of each flavor you need, and Combo and Zombie instances may simply refer to a flavor:

{ "KickPunch": { "name": "Kick Punch",
    "price": 2,
    "min_damage": 3,
    "max_damage": 9},
  "TripStomp": { ... },
  ....
}

State once-removed

You save your player's state in a @save hash inside your instance. This is not very object oriented, and doesn't seem to be warranted - facilitating a quick save is not a good enough reason. When you save, simply collect the relevant data.

It is also unclear what happens if the saved file does not exist when the game starts.

What does reset do?

Give clear names to your methods and members. What does @save_original hold? Why does reset change it? Why is it called when a player dies?

You have two different methods called attack which seem to do completely different stuff - one is a getter, while the other manages the battle. You might want to reconsider their names.

Model View Controller

Your design is not very MVC - the model prompts the user, and gives the user feedback, etc. Make an effort to separate the game logic from user interactions, it would make adapting your game to different UIs a lot easier, or should I say - possible.

When to use *args?

You use args quite liberally in your Cli class. args are used when you want to have a dynamic number of arguments. It is not a solution for a catch-all API. You want to tell the user of your API that he uses your API with too many arguments, rather than swallow it.

Cli.combo "some", ["garbage", "that"], :should_not, Be, here

I don't think that it is in the scope of CR to hunt down camelCase methods in your code, so I'm not going to do it (setInfo)

StackExchange Code Review Q#46983, answer score: 5