Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I got a PHP developer interview test to solve for a company. I didn't get the job but I would like to know where I was wrong. Did I understand the test completely?

/**
 * Create an Animal Choir simulator
 *
 * The task constraints are:
 *
 * There must be 3 different choir member animals
 * (i.e. dogs, cats, mice)
 *
 * Every animal must have a sing method that returns a string representation of a voice
 * (i.e. 'bark', 'meow', 'squeak')
 *
 * Every animal must have a loudness property which can have 3 settings,
 * depending on which the sing method result will be rendered as
 * lowercase, first letter uppercase and uppercase.
 *
 * Singer groups are groups of animals with the same loudness property value.
 * Singer group song is represented with a CSV of the group singer sing result in random order.
 *
 * The choir simulator must have implement the following methods:
 *    crescendo - the choir start singing from the least loud singer group, and then are being joined 
 *                by more and more loud singer groups until they are singing all together.            
 *                The joining is represented with a new line.
 *                Example:
 *                  meow, squeak, bark
 *                  Meow, bark, squeak, Bark, meow
 *                  bark, Meow, MEOW, squeak, BARK, meow, Bark
 *
 *    arpeggio  - the choir singer groups of the same loudness start singing one by one from
 *                the least loud to the loudest
 *                Example:
 *                  meow, squeak, bark
 *                  Meow, Bark
 *                  MEOW, BARK
 *
 */

//TODO: Describe the class hierarchy


//Choir class
class Choir{    

    public $line_ending = '';
    public $line_separator = '';
    public $voices = '';

    public function crescendo(){
        $crescendo_song = '';

        $animals = new Animal();        
        //We define which animal voices will be included in song
        $animals->voices = $this->voices;

        //First we start with the silent
        $animals->loudness = 'silent';
        //Call the function     
        $silent = $animals->sing();
        //echo the result
        $crescendo_song .= $this->stringForm($silent);

        //Then we continue with the normal, but also merge with silent
        $animals->loudness = 'normal';      
        $normal = array_merge($animals->sing(),$silent);
        $crescendo_song .= $this->stringForm($normal);

        //And in the end we merge loud with silent and normal
        $animals->loudness = 'loud';    
        $loud = array_merge($animals->sing(),$silent,$normal);
        $crescendo_song .= $this->stringForm($loud);

        return $crescendo_song;
    }


    public function arpeggio(){
        $arpeggio_song = '';

        $animals = new Animal();        
        //We define which animal voices will be included in song
        $animals->voices = $this->voices;

        //First we start with the silent
        $animals->loudness = 'silent';  
        $arpeggio_song .= $this->stringForm($animals->sing());

        //Then normal
        $animals->loudness = 'normal';      
        $arpeggio_song .= $this->stringForm($animals->sing());

        //And then the loud
        $animals->loudness = 'loud';
        $arpeggio_song .= $this->stringForm($animals->sing());

        return $arpeggio_song;

    }


    private function stringForm($array){
        //Randomize the array
        shuffle($array);
        //Predefine a song and a separator
        $song = '';
        $comma = '';
        //Form song string
        foreach ($array as $slog) {
            $song .= $comma.$slog;  
            $comma = $this->line_separator.' ';                 
        };
        return $song.$this->line_ending;
    }


}

//Animal class
class Animal{
    public $loudness = '';

    public function sing(){
        $song = array();
        foreach ($this->voices as $voices) {
            switch ($this->loudness) {
                case 'silent':
                    array_push($song,strtolower($voices));
                break;          
                case 'normal':
                    array_push($song,ucfirst($voices));
                break;
                case 'loud':
                    array_push($song,strtoupper($voices));
                break;
                default:
                    array_push($song,$voices);
                break;
            }
        }   
        return $song;
    }

}


$choir = new Choir();
$choir->line_ending = PHP_EOL;
//$choir->line_ending = '<br>'; //For cleared viewing in browser use <br> tag
//Define the line separator for CSV
$choir->line_separator = ',';
//Define the voices of animals
$choir->voices = array('bark','meow','squeak');
//Call and echo the functions
echo $choir->crescendo();
echo $choir->arpeggio();
share|improve this question
    
I would also ask the company what they think you can do to improve. –  Brett Santore Jul 23 at 19:25

2 Answers 2

up vote 12 down vote accepted

Let's start with class hierarchy. The choir is made up of the animals.

The Choir class should have a list of its Animal members:

$felix = new Cat();
$rex = new Dog();
$choir = new Choir(array($felix, $rex, $bernand));

Then each animal has a different way of singing but they all sing . That is classic polymorphism. (In fact they sing in such a similar way that the sing() method goes into the Animal class).

class Dog extends Animal {
    public function  __construct() {
        $noise = 'bark';
        ....

class Animal {
    private $loudness;
    private $loudnessLevels = array('silent', 'normal', 'screaming');
    private $noise;

    public function __set($name, $value) {
        if($name == 'loudness') {
            if(!in_array($value, $loudnessLevels) {
                throw new Excpetion('Unknown loudness');
            }

            $loudness = $value;
     ...

For a discussion about the value of getter/setters in PHP (coming from a C++/Python background I'm pleased never having to do PHP OOP).

Now for some (e.g. I didn't check for bugs) low-level criticism:

  • Use getters and setters
  • In your sing() function there is the default case - you just output the string in this case but actually it is an undefined case and should throw an exception.
share|improve this answer
    
Thank you, the thing I was apparently missing is that every animal can only have one sound(like cat can only meow, dog can only bark and so). For sing() function I just didn't know what to put by default so I set it to plain string.About getters and setters are you talking about constructor or magic methods? Thank you again. :) –  Aleksandar Andrijevic Jul 24 at 8:46

Unfortunately you seem to have misunderstood or missed this part of the brief:

There must be 3 different choir member animals (i.e. dogs, cats, mice)

Every animal must have a sing method that returns a string representation of a voice

So it sounds like they want it so you can add a mouse to the choir and it will only squeak, and a cat will only meow. Instead you have a single choir which randomly picks a sound to make and no distinct objects for the animals. I suspect they were wanting an Animal to make a single noise which is specified upfront in a more object-orientated way. Something like this at a minimum:

class /* enum */ Animals
{
    const CAT = 'meow';
    const DOG = 'bark';
    const MOUSE = 'squeak';
}

class /* enum */ Loudness
{
    const LOUD = 'loud';
    const NORMAL = 'normal';
    const SILENT = 'silent';
}

class SingingAnimal
{
    private $noise, $loudness;

    public function __construct($noise, $loudness)
    {
        $this->noise = $noise;
        $this->loudness = $loudness;
    }

    public function arpeggio()
    {
        //Implementation here
    }

    public function crescendo()
    {
        //Implementation here
    }

    public function sing()
    {
        //Implementation here
    }
}

class Choir
{
    public /* SingingAnimal[] */ $singers = array();

    public function addAnimal($animal, $loudness)
    {
        $this->singers[] = new SingingAnimal($animal, $loudness);
    }
}

$choir = new Choir();
$choir->addAnimal(AnimalNoises::CAT, Loudness::SILENT);
$choir->addAnimal(AnimalNoises::MOUSE, Loudness::LOUD);
$choir->addAnimal(AnimalNoises::DOG, Loudness::NORMAL);

Though even this is pretty sloppy, depending on the amount of time you have I'd probably go deeper and write out the standard interfaces and abstract classes they're looking for with these tests: interface Animal, abstract class Enum (for all the validation methods), class LoudnessEnum extends Enum, class AnimalEnum extends Enum, etc.

share|improve this answer
    
Thank you very much :) –  Aleksandar Andrijevic Jul 24 at 8:48

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.