5
\$\begingroup\$

Originally asked on Stack Overflow

I recently had an interview where interviewers asked me to

  1. Rewrite following code with SOLID principles.
  2. Add a new file handler (PriceFileHandler for example).
  3. Add supporting stream parsing (to one parser implementation)

Here is the code which I had to change:

<?php

class FileHandler
{
    public function parse(string $type, string $data)
    {
        switch ($type) {
            case 'xml':
                $parser = new XmlParser();
                break;
            case 'csv':
                $parser = new CsvParser();
                break;
            default:
                throw new InvalidArgumentException();
        }
        return $parser->parse($data);
    }
}

class XmlParser
{
    /**
     * @param string $data
     * @return SimpleXMLElement
     */
    public function parse(string $data)
    {
        // ...
    }
}

class CsvParser
{
    /**
     * @param string $data
     * @return string
     */
    public function parse(string $data)
    {
        // ...
    }
}

as result I rewrite it to following code:

<?php

interface FileHandlerInterface
{
    /**
     * @param string $type
     * @param string $data
     *
     * @return array >>>> always return parsed data in one format
     */
    public function parseString(string $type, string $data);

    /**
     * @param string   $type
     * @param resource $data
     *
     * @return array >>>> always return parsed data in one format
     */
    public function parseStream(string $type, resource $data);
}

abstract class AbstractFileHandler implements FileHandlerInterface
{
    /**
     * @var ParserInterface[]
     */
    private $parsers = [];

    public function addParser(ParserInterface $parser)
    {
        $this->loaders[$parser->getAlias()] = $parser;
    }

    /**
     * @param string $alias
     * @return StringParserInterface
     * @throws Exception
     */
    public function getStringParser(string $alias)
    {
        if (!isset($this->parsers[$alias])) {
            throw new Exception;
        }
        if (!($this->parsers[$alias] instanceof StringParserInterface)) {
            throw new Exception;
        }
        return $this->parsers[$alias];
    }

    /**
     * @param string $alias
     * @return StreamParserInterface
     * @throws Exception
     */
    public function getStreamParser(string $alias)
    {
        if (!isset($this->parsers[$alias])) {
            throw new Exception;
        }
        if (!($this->parsers[$alias] instanceof StreamParserInterface)) {
            throw new Exception;
        }
        return $this->parsers[$alias];
    }
}

class PriceFileHandler extends AbstractFileHandler
{
    /**
     * @param string $type
     * @param string $data
     *
     * @return array
     */
    public function parseString(string $type, string $data)
    {
        $parser = $this->getStringParser($type);
        $parsedData = $parser->parse($data);

        // remove items with price <= 0
        foreach ($parsedData as $key => $parsedItem) {
            if ($parsedItem['price'] <= 0) {
                unset($parsedData[$key]);
            }
        }

        return $parsedData;
    }

    /**
     * @param string   $type
     * @param resource $data
     *
     * @return array
     */
    public function parseStream(string $type, resource $data)
    {
        throw new Exception('doesn\'t support');
    }
}

class FileHandler extends AbstractFileHandler
{
    /**
     * @param string $type
     * @param string $data
     *
     * @return array
     */
    public function parseString(string $type, string $data)
    {
        $parser = $this->getStringParser($type);

        return $parser->parseString($data);
    }

    /**
     * @param string   $type
     * @param resource $data
     *
     * @return array
     */
    public function parseStream(string $type, resource $data)
    {
        $parser = $this->getStreamParser($type);

        return $parser->parseStream($data);
    }
}

interface ParserInterface
{
}

interface StringParserInterface
{
    public function parseString(string $data);
}

interface StreamParserInterface
{
    public function parseStream(resource $data);
}

class XmlParser implements ParserInterface, StringParserInterface, StreamParserInterface
{
    /**
     * @param string $data
     * @return array
     */
    public function parseString(string $data)
    {
        // ...
    }

    /**
     * @param resource $data
     * @return array
     */
    public function parseStream(resource $data)
    {
        // ...
    }
}

class CsvParser implements ParserInterface, StringParserInterface
{
    /**
     * @param string $data
     * @return array
     */
    public function parseString(string $data)
    {
        // ...
    }
}

The interviewers said I broke these principles:

  • SRP - perhaps about parsing from different sources in file handlers....
  • LSP - ....
  • OCP - ....

Could someone describe where and why I've made mistakes or what I've done wrong and how to do it in correct way?

I've read and re-read principles with examples and don't see where I could broke LSP and OCP principles :(

\$\endgroup\$
3
\$\begingroup\$

SRP

As you noticed too, this one is fairly clear, your grouping of different sources of data under a single parser interface violates the single responsibility principle. Each different source type should be split up to have its own interface.

LSP

LSP has been covered extensively in this SO post - which I advise that you take a look through. I have no intention to regurgitate what has previously been written, so again, I'll keep it brief. Liskov imposes certain standards, one of them being:

No new exceptions should be thrown by methods of the subtype, except where those exceptions are themselves subtypes of exceptions thrown by the methods of the supertype.

When you define the function parseStream in your interface FileHandlerInterface:

/**
 * @param string   $type
 * @param resource $data
 *
 * @return array >>>> always return parsed data in one format
 */
public function parseStream(string $type, resource $data);

The return is specified as an array, it actually says 'always return parsed data in one format', so it seems that you were consciously aware of the principle but maybe in the heat of the moment you glanced over it further on. If we look in the PriceFileHandler class, you'll notice that you define the function with nothing but an exception as part of its body:

/**
 * @param string   $type
 * @param resource $data
 *
 * @return array
 */
public function parseStream(string $type, resource $data)
{
    throw new Exception('doesn\'t support');
}

Yet, the return type that is documented for this definition is an array too. This falls under the undesired effects category that LSP warns against.

As far as OCP goes, I can't see anything right now, however, I'll revisit this again if I do see something.

\$\endgroup\$
1
\$\begingroup\$

This question does not contain working code, a requirement of Code Review, and is more about programming principles. This is still an interesting question though.

I have to admit I don't know much about SOLID principles, but I have been programming for decades, so these principles do make some sense to me. They are basic steps towards better code. These principles alone do, however, not produce good code. They are very abstract, and might be difficult to implement in practice. In my opinion code should first and foremost be easy to read/understand, and suited to the task at hand.

So, let's first, before we refactor the code, think about what the code is supposed to do. That is not very clear. Perhaps there was a written introduction accompanying the code, which has been left out of this question? We have a class called FileHandler which parses data, probably from a file, using two different parser classes. That's very little information. The name of the FileHandler class doesn't even cover what it actually does. It doesn't actually handle any file. On the other hand, the vagueness gives us a lot of freedom.

The first thing that is clearly wrong with the code is the way the parser classes are handled. Instead of handing over a certain parse class to the file handler, a $type parameter is given to the the parse() method. This probably breaks several of the SOLID principles. In my own simple words: Whenever a new parser class is created, the file handler class needs to be edited to make use of it. Let us therefore first correct this problem.

class FileHandler
{
    public function __construct(string $filename, ParserInterface $parser)
    {
        $this->filename = $filename;
        $this->parser = $parser;
    }

    public function parse(string $data)
    {
        return $parser->parse($data);
    }
}

interface ParserInterface
{
     public function parse(string $data)
}

class XmlParser implements ParserInterface
{
    public function parse(string $data)
    {
    }
}

class CsvParser implements ParserInterface
{
    public function parse(string $data)
    {
    }
}

Now this code looks a lot cleaner, and adheres, I hope, to the SOLID principles. Step 1 done. I left out all comments to shorten the code, but obviously you would add them in the real code.

Now I didn't do much with the FileHandler class yet. That's because this is part of the next step: We should add a new file handler called PriceFileHandler. It would make sense to create a file handler interface and implement two handlers. Like this:

interface FileHandlerInterface
{
    public function __construct(string $filename, ParserInterface $parser)
    public function parse(string $data)
}

class FirstFileHandler implements FileHandlerInterface
{
    public function __construct(string $filename, ParserInterface $parser)
    {
        $this->filename = $filename;
        $this->parser = parser;
    }

    public function parse(string $data)
    {
        return $parser->parse($data);
    }
}

class PriceFileHandler implements FileHandlerInterface
{
    public function __construct(string $filename, ParserInterface $parser)
    {
        $this->filename = $filename;
        $this->parser = parser;
    }

    public function parse(string $data)
    {
        return $parser->parse($data);
    }
}

I agree that this looks boring, but then again, we have no idea yet, what to do with a file containing prices. This is abstract and incomplete code. There's only so much we can do before the code becomes completely unrecognizable.

Finally we have to add 'supporting stream parsing to one parser implementation'. A stream in PHP is a resource object which exhibits streamable behavior. See: https://www.php.net/manual/en/intro.stream.php So, given a stream, the parser class should parse it. Let's use the method parseStream() for this.

class XmlParser implements ParserInterface
{
    public function parse(string $data)
    {
    }

    public function parseStream(string $filename)
    {
        $data = file_get_contents($filename);
        return $this->parse($data);
    }
}

Is that all? Well, yeah. In the end, hidden inside PHP, streams are used by functions like file_get_contents(), so I used that. If the files get very big you would need to read the file in manageble chunks. That would require quite a bit more code than this question warrants. To be honest, I don't think this would be a good answer to the assignment.

By now we should have notice that if we want to make use of the new parseStream() method we would have to extend a file handler, and probably the interfaces. It might make more sense to give a parser an abstract file handler, and let it parse that, than the other way around. That way we don't need a parser() method in the file handler at all, and it can do what it should be doing: handling a file. These classes would then better adhere to the Single Responsibility Principle (SRP). I think doing this here would be beyond the scope of this question. We have to work with what was given: Not much.

I do realise that this answer doesn't answer all of your questions. It is how I would approach the code at hand. Correct coding requires deep knowledge of the domain it is applied to. Should the file handlers use a parser or should the parsers use a file handle? This depends on how the rest of the code will be structured. Normally it would be weird for file handlers to do the parsing, but what if that was the whole point of handling the files? In other words, there is no 100% correct solution to this assignment.

The two follow up questions are difficult to answer, and they imply a certain solution to the first question. Perhaps the assignment is not a very good one?

Priciples are often quite abstract, and therefore somewhat difficult to understand. Seeing refactoring, and my thought process, in action might help you. So instead of commenting on your code I made my own. I hope you find this helpful.

\$\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.