7
\$\begingroup\$

How idiomatic is my code? Since Lua does not have classical inheritance such as in OO languages. Feedback on the test and implementation are both appreciated/welcomed.

The rules are:

  • If the phone number is less than 10 digits assume that it is bad number
  • If the phone number is 10 digits assume that it is good
  • If the phone number is 11 digits and the first number is 1, trim the 1 and use the first 10 digits
  • If the phone number is 11 digits and the first number is not 1, then it is a bad number
  • If the phone number is more than 11 digits assume that it is a bad number

local PhoneNumber = {}

function PhoneNumber:new(no_as_string)
  self.__index = self
  local n = "0000000000"
  n = no_as_string:gsub("[^0-9]", "")
  if (n:len() == 11 and n:sub(1, 1) == "1") then 
    n = n:sub(2, 11)
  else 
    if (n:len() > 10 or n:len() < 10) then
      n = "0000000000"
    end
  end
  return setmetatable({ number = n }, self)
end

function PhoneNumber:areaCode(symbol)
  return self.number:sub(1, 3)
end

function PhoneNumber:toString(symbol)
  return "("..self.number:sub(1, 3)..") "..self.number:sub(4, 6).."-"..self.number:sub(7, 10)
end

return PhoneNumber

Here is the test:

local PhoneNumber = require('phone-number')

describe("PhoneNumber()", function() 
  it("cleans the number (123) 456-7890", function() 
    local phone = PhoneNumber:new("(123) 456-7890")
    assert.are.equals(phone.number,"1234567890")
  end)

  it("cleans numbers with dots", function()
    local phone = PhoneNumber:new("123.456.7890")
    assert.are.equals(phone.number,"1234567890")
  end)

  it("valid when 11 digits and first digit is 1", function()
    local phone = PhoneNumber:new("11234567890")
    assert.are.equals(phone.number,"1234567890")
  end)

  it("invalid when 11 digits", function()
    local phone = PhoneNumber:new("21234567890")
    assert.are.equals(phone.number,"0000000000")
  end)

  it("invalid when 9 digits", function()
    local phone = PhoneNumber:new("123456789")
    assert.are.equals(phone.number,"0000000000")
  end)

  it("has an area code", function()
    local phone = PhoneNumber:new("1234567890")
    assert.are.equals(phone:areaCode(),"123")
  end)

  it("formats a number", function()
    local phone = PhoneNumber:new("1234567890")
    assert.are.equals(phone:toString(),"(123) 456-7890")
  end)
end)
\$\endgroup\$
0
4
\$\begingroup\$

You've a lot of unnecessary code. First off, as janos stated:

local n = "0000000000"
n = no_as_string:gsub("[^0-9]", "")

The initialization of n is pointless here.

You can instead use just the lua pattern to match your input to desired result:

function PhoneNumber:new( Number )
    local n = string.gsub( Number, "%D", "" )
    n = n:match "^1?(%d%d%d%d%d%d%d%d%d%d)$" or "0000000000"
    return setmetatable({ number = n, __index = self }, self)
end

Notice that I've changed the input argument to Number and later use string.gsub( Number.... This way; you've the advantage of inputting direct numbers and provide flexibility.

The pattern

"^1?(%d%d%d%d%d%d%d%d%d%d)$"

is self-explanatory. In lua, if string.match failed to find any matched group; it returns nil. I've used this property to check for bad numbers.


Next thing I'd suggest that you do is store the number as an integer instead of string:

return setmetatable({ number = tonumber(n), __index = self }, self)

This is just because integers take less memory than strings. For retrieving area code etc., use mathematical operations:

function PhoneNumber:areaCode()  -- what was symbol parameter for?
    return math.floor( self.number / 10^7 )
end

Lastly, lua provides a meta-method: __tostring for conversion to strings from other types. Use it (It is the reason why I was able to use string.gsub above):

function PhoneNumber:__tostring()
    local f, n = math.floor, self.number
    local sFormat, iPart1, iPart2 = "(%d) %d-%d", f( n / 10^7 ), f( n / 10^3 % 10^4 )
    return sFormat:format( self:areaCode(), iPart1, iPart2 )
end

Now, instead of calling PhoneObject:toString(), you can do:

tostring( PhoneObject )
\$\endgroup\$
1
  • \$\begingroup\$ So many concepts explained. -I had been wondering how to use tostring in lua - string.gsub vs string:gsub - also the regex is sweet. -I did not even know abt the format function :) \$\endgroup\$
    – aarti
    Nov 25 '14 at 19:42
3
\$\begingroup\$
  local n = "0000000000"
  n = no_as_string:gsub("[^0-9]", "")

The initialization of n is pointless here. You should combine these two lines.


  if (n:len() == 11 and n:sub(1, 1) == "1") then 
    n = n:sub(2, 11)
  else 
    if (n:len() > 10 or n:len() < 10) then
      n = "0000000000"
    end
  end

Using an elseif for the second if would reduce a level of nesting and be shorter and simpler.


    if (n:len() > 10 or n:len() < 10) then

Instead of checking if len is bigger than 10 or smaller than 10, it would be simpler to check equality with 10. That would also eliminate one unnecessary call to len(). That is:

    if (n:len() != 10) then

assert.are.equals(phone.number,"1234567890")

In unit testing, the convention in many languages is to put the expected value first, and the actual value second in assert statements. So I suggest to flip the arguments.

\$\endgroup\$

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.