1
\$\begingroup\$

I have the following code (in php) that calls the open weather api with my credentials, and returns data for me.

Know I'm returning some data I pick from that response, I'm wondering if it's good practice to create a dedicated class for that? Is this something that's commonly used and does it have a name?

try {
    $request = $this->http->get( 'api.openweathermap.org/data/2.5/weather', $options);
    $response = json_decode($request->getBody());

    return [
        'main' => $response->weather[0]->main,
        'temperature' => $response->main->temp,
    ];

} catch (GuzzleException $e) {
    die(var_dump($e->getMessage()));
}
```
\$\endgroup\$
1
  • 2
    \$\begingroup\$ You might want to include the entire class. And in pure OOP, everything is a class, So generally, the answer is yes. In this particular case, I'd call it adapter. \$\endgroup\$
    – slepic
    Sep 7, 2020 at 4:31

1 Answer 1

2
\$\begingroup\$

Your might actually take it a step further and create a value object for the data, making it much more type safe. Notice I added PHP 7.4 proeprty typehints. I chosen float for the temperature (maybe it's not appropriate, idk), but i definitely don't know what to choose for the main, but it sure deserves a typehint, if it's nested object, create another class for it or maybe pick the wanted information directly into the WeatherStatus class.

final class WeatherStatus
{
  private ??? $main;
  private float $temperature;

  public function __construct(??? $main, float $temperature)
  {
    $this->main = $main;
    $this->temperature = $temperature;
  }

  public function getMain(): ???
  {
    return $this->main;
  }

  public function getTemperature(): float
  {
    return $this->temperature;
  }
}

You can also define an interface for such a method, including a domain specific exception (because just halting the program with die or exit is not very nice thing to do, in that case it would be better to not catch the exception at all).

class WeatherProviderException extends \Exception
{
}

interface IWeatherProvider
{
  /**
   * @throws WeatherProviderException
   */
  public function getWeather(): WeatherStatus;
}

In the implementation I would accept the api url rather then hardcoding it. You may add a static named constructor for the version 2.5 api. The credentials for openweathermap.org (whatever they are, let me assume a user name and password) might be also promoted to a class or may be passed to the provider constructor as multiple arguments as well.

final class OpenWeatherCredentials
{
  private string $user;
  private string $password;

  // constructor + getters ...
}
class OpenWeatherProvider implements IWeatherProvider
{
  private ClientInterface $client;
  private string $apiUrl;
  private OpenWeatherCredentials $credentials;

  public function __construct(ClientInterface $client, string $apiUrl, OpenWeatherCredentials $credentials)
  {
    $this->client = $client;
    $this->apiUrl = $apiUrl;
    $this->credentials = $credentials;
  }

  public static function createV2_5(ClientInterface $client, OpenWeatherCredentials $credentials): self
  {
    return new self($client, 'https://api.openweathermap.org/data/2.5/weather', $credentials);
  }

  public function getWeather(): WeatherStatus
  {
    $options = [
      // whatever is needed, $this->credentials->get*
    ];

    try {
      // here you called the object $request, but it really is a $response
      $response = $this->client->get($this->apiUrl, $options);
    } catch (GuzzleException $e) {
      throw new WeatherProviderException($e->getMessage(), $e->getCode(), $e);
    }

    try {
      $json = json_decode($response->getBody(), \JSON_THROW_ON_ERROR);
    } catch (\JsonException $e) {
      throw new WeatherProviderException($e->getMessage(), $e->getCode(), $e);
    }

    return new WeatherStatus(
      $json->weather[0]->main,
      (float) $json->main->temp,
    );
  }
}

Also notice how I wrap each statement in separate try-catch to only catch what can be caught. As long as they are handled the same way you try-catch both together but maybe you should catch their common ancestor exception instead to make it even simpler. And I pulled the instantiation of the WeatherStatus out of the try-catch just because I don't expect it to throw GuzzleException nor \JsonException.

\$\endgroup\$
2
  • \$\begingroup\$ This is great! Very interesting approach, I suppose this follows ddd principles? Thank you! \$\endgroup\$ Sep 13, 2020 at 8:43
  • 1
    \$\begingroup\$ @MiguelStevens Well, I guess you could say so. But to me, ddd is just a buzz word. I'd rather say it follows common sense and top to bottom approach. First define what you need to be done (get current weather status), then implement it using whatever you found appropriate (openweathermap.org accessed through http). The separation of domains is just a side effect of application of common sense. Did you ever heard a weather forecaster to tell you "tommorow will be.. HTTP 500", those things just don't belong together, in TV they would just say "we have technical difficulties" \$\endgroup\$
    – slepic
    Sep 14, 2020 at 5:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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