Convert Number to Textual Equivalent

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Re: Convert Number to Textual Equivalent

Post by VladSun »

I have a bit of criticism for your OOP version... Your class seems to be stateless (which is fine), so it must be a static one - no constructors, no "new" operators, just one single (and static) method - WordedNumber::read(int $value, ...)
There are 10 types of people in this world, those who understand binary and those who don't
josh
DevNet Master
Posts: 4872
Joined: Wed Feb 11, 2004 3:23 pm
Location: Palm beach, Florida

Re: Convert Number to Textual Equivalent

Post by josh »

VladSun wrote:I have a bit of criticism for your OOP version... Your class seems to be stateless (which is fine), so it must be a static one - no constructors, no "new" operators, just one single (and static) method - WordedNumber::read(int $value, ...)
It does too have a constructor & state. We'll agree to disagree. Its not perfect but its pretty darn good for his first class ;-) Something like this doesn't have to use state either. Its just a calculator. Even the java math library is static and a lot of people use that.

Its good that he did he did use state though, it makes it modular.
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Re: Convert Number to Textual Equivalent

Post by VladSun »

I said a "bit of criticism", so I made myself clear it was not so important.

But ... having a several private methods, one public method (which does nothing) and a public constructor, makes a class a pure static one, right? In this case you can't have integer values "worded" without creating an instance every time you need it. That's not good.
There are 10 types of people in this world, those who understand binary and those who don't
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Re: Convert Number to Textual Equivalent

Post by Benjamin »

Wait what? Where's the multi language support?
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Re: Convert Number to Textual Equivalent

Post by VladSun »

Benjamin wrote:Wait what? Where's the multi language support?
:lol:

josh wrote:Its good that he did he did use state though, it makes it modular.
I'm not sure I got that one.

Indeed, I think that passing the value to be converted to the constructor is the wrong step to make in this case.

It's

Code: Select all

$var = new WordedNumber(mixed $number1 [, boolean $commas_enabled defaults true]);
echo $var->read();

$var = new WordedNumber(mixed $number2 [, boolean $commas_enabled defaults true]);
echo $var->read();

$var = new WordedNumber(mixed $number3 [, boolean $commas_enabled defaults true]);
echo $var->read();
vs.

Code: Select all

$var = new WordedNumber();
echo $var->read(mixed $number1 [, boolean $commas_enabled defaults true]);
echo $var->read(mixed $number3 [, boolean $commas_enabled defaults true]);
echo $var->read(mixed $number2 [, boolean $commas_enabled defaults true]);
or

Code: Select all

echo WordedNumber::read(mixed $number1 [, boolean $commas_enabled defaults true]);
echo WordedNumber::read(mixed $number2 [, boolean $commas_enabled defaults true]);
echo WordedNumber::read(mixed $number3 [, boolean $commas_enabled defaults true]);
or

Code: Select all

$var = new WordedNumber();

$var->set(mixed $number1 [, boolean $commas_enabled defaults true]);
echo $var->read();

$var->set(mixed $number2 [, boolean $commas_enabled defaults true]);
echo $var->read();

$var->set(mixed $number3 [, boolean $commas_enabled defaults true]);
echo $var->read();
well, I still like the static version most.
There are 10 types of people in this world, those who understand binary and those who don't
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: Convert Number to Textual Equivalent

Post by Jonah Bron »

Benjamin wrote:Wait what? Where's the multi language support?
Change the words in the array. :D

While I agree that the static approach makes thing a bit easier on the other end, In your version it sort of clumped everything back into one-ish function, and it seems like that defeats the purpose of putting it into a class.
VladSun wrote:...It seemed to be very elegant but it failed on several tests...
Just to lazily save me the trouble of testing it, are you saying that the one you gave works or not?
VladSun wrote:I think that passing the value to be converted to the constructor is the wrong step to make in this case
Just for fun, here's the patch. (works if there are two new lines before beginning of class)
[text]16c16,17
< public function __construct($int, $commas=true) {
---
> public function read($int, $commas=true) {
> $this->text = '';
29a31
> return $this->text;
80,84d81
<
< // Pass result up
< public function read() {
< return $this->text;
< }[/text]
For the more boring types (or those on *sneer* Windows and don't have patch), here's the whole thing. :D

Code: Select all

class WordedNumber {
	private $number;
	private $text;
	private $w_ones = array('zero', 'one', 'two', 'three', 'four', 'five', 'six', 'seven', 'eight', 'nine');
	private $w_tens = array(null, null, 'twenty', 'thirty', 'fourty', 'fifty', 'sixty', 'seventy', 'eighty', 'ninty');
	private $w_teens = array('ten', 'eleven', 'twelve', 'thirteen', 'fourteen', 'fifteen', 'sixteen', 'seventeen', 'eighteen', 'ninteen');
	private $w_name = array('', ' thousand', ' million', ' billion', ' trillion', ' quadrillion');
	private $comma_index = 0;
	private $name_index = false;
	private $digits = 0;
	private $comma_enabled;
	
	// Parent processor
	public function read($int, $commas=true) {
		$this->text = '';
		if (intval($int) == 0) {
			$this->text = $this->w_ones[0];
			return;
		}
		$this->formatInput($int);
		$this->setNamePos();
		$this->comma_enabled = $commas;
		foreach ($this->number as $chunk) {
			$chunk = array_pad($chunk, 3, 0);
			$tens = $this->getTens($chunk);
			$hundreds = $this->getHundreds($chunk[2]);
			$this->text .= $hundreds . $tens;
		}
		return $this->text;
	}
	
	// Process first two digits
	private function getTens($int) {
		$have_comma = ($this->comma_index > 1 && $this->comma_enabled ? ', ' : ' ');
		$len = count($int);
		$name = prev($this->w_name);
		$result = '';
		if ($int[1] == 1) {
			$teen_num = $this->w_teens[$int[0]];
			$result = $teen_num . $name . $have_comma;
			$this->comma_index++;
			$this->name_index = true;
		} elseif ($int[0] > 0 || $int[1] > 0) {
			$one_digit = $this->w_ones[$int[0]];
			$ten_digit = $this->w_tens[$int[1]];
			$have_dash = $ten_digit && $one_digit ? '-' : '';
			$result = $ten_digit . $have_dash . $one_digit . $name . $have_comma;
			$this->comma_index++;
			$this->name_index = true;
		}
		return $result;
	}
	
	// Process hundred's place
	private function getHundreds($int) {
		$result = '';
		if ($int > 0) {
			$one_digit = $this->w_ones[$int];
			$num_name = ($this->name_index ? '' : prev($this->w_name));
			$have_comma = ($this->comma_index > 1 && !$this->name_index && $this->comma_enabled ? ', ' : ' ');
			$result = $one_digit . ' hundred' . $num_name . $have_comma;
			$this->comma_index++;
		}
		return $result;
	}
	
	// Get input ready for processing
	private function formatInput($int) {
		$this->number = array_map('intval', array_reverse(str_split(strval($int))));
		$this->digits = count($this->number);
		$this->number = array_reverse(array_chunk($this->number, 3));
	}		
	
	// Set initial index position for number names
	private function setNamePos() {
		for ($i = 0; $i < count($this->number); $i++) {
			next($this->w_name);
		}
	}
}
User avatar
VladSun
DevNet Master
Posts: 4313
Joined: Wed Jun 27, 2007 9:44 am
Location: Sofia, Bulgaria

Re: Convert Number to Textual Equivalent

Post by VladSun »

Jonah Bron wrote:While I agree that the static approach makes thing a bit easier on the other end, In your version it sort of clumped everything back into one-ish function, and it seems like that defeats the purpose of putting it into a class.
You've put everything into the constructor in the very same way ;)
By using a pure static class I've managed to encapsulate two functions, two numeric constants and four array constants.
Of course you can always rewrite it in a procedural way by using a function in function (even in your version), but I think it will be harder to write and read the code.

And the last thing to mention - the intToWords method may extend the Integer object if it was possible ;)
Jonah Bron wrote:
VladSun wrote:...It seemed to be very elegant but it failed on several tests...
Just to lazily save me the trouble of testing it, are you saying that the one you gave works or not?
Sorry, I was talking about my recursive version attempt.
There are 10 types of people in this world, those who understand binary and those who don't
User avatar
AbraCadaver
DevNet Master
Posts: 2572
Joined: Mon Feb 24, 2003 10:12 am
Location: The Republic of Texas
Contact:

Re: Convert Number to Textual Equivalent

Post by AbraCadaver »

I was bored and it seemed like fun so I wrote it procedural style after seeing the first post by Jonah Bron, and then took VladSun's advice and converted to a static class:

Code: Select all

function number_to_words($string, $sep=', ') {
	$singles = array('','one','two','three','four','five','six','seven','eight','nine');
	$teens = array('','eleven','twelve','thirteen','fourteen','fifteen','sixteen','seventeen','eightteen','nineteen');
	$doubles = array('','ten','twenty','thirty','forty','fifty','sixty','seventy','eighty','ninety');
	$extended = array('','thousand','million','billion','trillion','quadrillion','quintillion','sextillion',
                                   'septillion','octillion','nonillion','decillion');
	
	$groups = array_reverse(array_map('strrev', array_filter(str_split(strrev($string), 3))));
	
	foreach($groups as $key => $group) {
		$len = strlen($group);
		$text = array();
		
		switch($len) {			
			case 3;
				if($group[$len-3] > 0) {
					$text[] = $singles[$group[$len-3]];
					$text[] = 'hundred';
				}
				
			case 2:
				if($group[$len-2] == 1 && $group[$len-1] > 0) {
					$text[] = $teens[$group[$len-1]];
					break;
				} else {
					$text[] = $doubles[$group[$len-2]];
				}
				
			case 1:
				$text[] = $singles[$group[$len-1]];						
		}
		$text[] = $extended[count($groups)-$key-1];
		$return[] = implode(' ', array_filter($text));
	}
	return implode($sep, $return);
}

Code: Select all

class NumberWords {
	
	public static $seperator = ', ';
	private static $singles = array('','one','two','three','four','five','six','seven','eight','nine');
	private static $teens = array('','eleven','twelve','thirteen','fourteen','fifteen','sixteen','seventeen','eightteen','nineteen');
	private static $doubles = array('','ten','twenty','thirty','forty','fifty','sixty','seventy','eighty','ninety');
	private static $extended = array('','thousand','million','billion','trillion','quadrillion','quintillion','sextillion',
                                                       'septillion','octillion','nonillion','decillion');
	private static $result;
	
	public static function convert($string) {
		if(!is_numeric($string)) {
			return false;
		}
		$groups = array_reverse(array_map('strrev', array_filter(str_split(strrev($string), 3))));
		array_walk($groups, array('self', 'parse_group'), count($groups));
		
		return implode(self::$seperator, self::$result);
	}
	
	private static function parse_group($group, $key, $count) {
		$len = strlen($group);
		
		switch($len) {			
			case 3;
				if($group[$len-3] > 0) {
					$text[] = self::$singles[$group[$len-3]];
					$text[] = 'hundred';
				}
				
			case 2:
				if($group[$len-2] == 1 && $group[$len-1] > 0) {
					$text[] = self::$teens[$group[$len-1]];
					break;
				} else {
					$text[] = self::$doubles[$group[$len-2]];
				}
				
			case 1:
				$text[] = self::$singles[$group[$len-1]];						
		}
		$text[] = self::$extended[$count-$key-1];		
		self::$result[] = implode(' ', array_filter($text));
	}
}
mysql_function(): WARNING: This extension is deprecated as of PHP 5.5.0, and will be removed in the future. Instead, the MySQLi or PDO_MySQLextension should be used. See also MySQL: choosing an API guide and related FAQ for more information.
User avatar
Jonah Bron
DevNet Master
Posts: 2764
Joined: Thu Mar 15, 2007 6:28 pm
Location: Redding, California

Re: Convert Number to Textual Equivalent

Post by Jonah Bron »

VladSun wrote:You've put everything into the constructor in the very same way...
Wasn't it split up into several functions? Or are you referring to the fact that all of the others are private?

@AbraCadaver, way to think outside of the box :) . That's a pretty cool trick with the switch statement; it took me 5 minutes just to figure out how it worked. That's considerably shorter than mine. It's so short in fact that there's not really much point to putting it into a class: static or otherwise.
josh
DevNet Master
Posts: 4872
Joined: Wed Feb 11, 2004 3:23 pm
Location: Palm beach, Florida

Re: Convert Number to Textual Equivalent

Post by josh »

Jonah Bron wrote: That's a pretty cool trick with the switch statement; it took me 5 minutes just to figure out how it worked. That's considerably shorter than mine.
Also less readable :x If you want your code to be modular you should use a class. You shouldn't expect people to have to edit the code directly because then they can't upgrade to your new versions of that code very easily without loosing the modifications they had made themselves. You can't call yourself modular and expect me to ninja-merge the code when you release updates :( I am of course talking about Object Inheritance

And yeah those "static" rewrites that (Vlad?) suggested destroys any shred of extensibility you had going, in my opinion. If you want I could get in to detail on why that is, but it would be best to start another thread at that point.

The fact it took you 5 minutes just to figure out what the compiler is doing means it is crap code, in general. Well in the larger scheme of things its still good code, but I wouldn't call it good code really. Good code leaves you with a "wow that was easy to understand" feeling. Being hard to understand is not an attribute of good code, its an attribute of bad code. You're writing a computer program not a mystery novel, lol.
User avatar
AbraCadaver
DevNet Master
Posts: 2572
Joined: Mon Feb 24, 2003 10:12 am
Location: The Republic of Texas
Contact:

Re: Convert Number to Textual Equivalent

Post by AbraCadaver »

josh wrote:
Jonah Bron wrote: That's a pretty cool trick with the switch statement; it took me 5 minutes just to figure out how it worked. That's considerably shorter than mine.
Also less readable :x If you want your code to be modular you should use a class. You shouldn't expect people to have to edit the code directly because then they can't upgrade to your new versions of that code very easily without loosing the modifications they had made themselves. You can't call yourself modular and expect me to ninja-merge the code when you release updates :( I am of course talking about Object Inheritance

And yeah those "static" rewrites that Vlad suggested destroys any shred of extensibility you had going, in my opinion. If you want I could get in to detail on why that is, but it would be best to start another thread at that point.

The fact it took you 5 minutes just to figure out what the compiler is doing means it is crap code, in general. Well in the larger scheme of things its still good code, but I wouldn't call it good code really. Good code leaves you with a "wow that was easy to understand" feeling. Being hard to understand is not an attribute of good code, its an attribute of bad code. You're writing a computer program not a mystery novel, lol.
Blah, blah, blah... But thanks for looking. :wink:
mysql_function(): WARNING: This extension is deprecated as of PHP 5.5.0, and will be removed in the future. Instead, the MySQLi or PDO_MySQLextension should be used. See also MySQL: choosing an API guide and related FAQ for more information.
Post Reply