Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I've created the following string manipulation function for randomizing my passed string in Lua:

require "string"
require "math"

math.randomseed( os.time() )

function string.random( self )
    local tTemporary, tNew = {}, {}
    if not self or self:len() < 5 then
        return nil
    end
    self:gsub( "%a", function( cChar )
            table.insert( tTemporary, cChar )
        end
    )
    for i = 1, #tTemporary, 1 do
        local iRandom = math.random(1, #tTemporary)
        tNew[i] = tTemporary[iRandom]
        table.remove( tTemporary, iRandom )
    end
    return table.concat(tNew, "   ")
end

Can this be optimized/more-randomised?

share|improve this question
add comment

1 Answer

up vote 2 down vote accepted

A couple of points to consider:

  • Calling your function shuffle instead of random would be a better name since that's really what it's doing.
  • The extra intermediate tables aren't really necessary to perform the shuffle. A single table is enough and you can perform the shuffle in-place right on that table.
  • Consider a better separation between the process of shuffling and how the input is stored. For example, if you have your shuffle function take a table instead then that function can be reused on any kind of data and not just strings.
  • I would do away with the Hungarian notation and just drop the type prefix on the variable names. Remember Lua variables themselves doesn't have a type. If you later change what that variable refers to then the Hungarian notation becomes misleading.

So I would refactor your original code into two functions. Here's one possibility:

function shuffle(self)
  if type(self) ~= 'table' then return nil end

  for i = #self, 2, -1 do
    local randi = math.random(i)
    self[i], self[randi] = self[randi], self[i]
  end
  return self
end

function shuffle_words(str)
  local strtable = {}
  for each in str:gmatch("%a+") do
    table.insert(strtable, each)
  end
  return table.concat(shuffle(strtable), ' ')
end
share|improve this answer
 
"%a+" should be "%a" –  hjpotter92 May 4 at 5:58
 
This is just an example of course, it assumes the input string is a bunch of words delimited by whitespace. If you have a different set of constraints then yes you should change the regex pattern to fit your needs. –  greatwolf May 5 at 20:47
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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