Importing data from spreadsheet

Question

Not really an issue I am stuck on (the code works) but I just really don't like some code I wrote, if you're bored in lockdown maybe you might like to help.

Context: I am import some data from a spreadsheet, it contains data about retail stores (grocery stores). As the data comes in from the spreadsheet, each row is stored as a StoreData object. Each store has upto five tills (point of sale devices) and I want the StoreData class to be able to return an array of information about each till. Here is the class showing the code relevant to the tills (the class is much bigger).

How can I clean up the function getTills()?

class StoreData
{
    private $spreadsheetData;

    private $tillModel;
    private $tillVersion;
    private $tillOneName;
    private $tillOneIpAddress;
    private $tillTwoName;
    private $tillTwoIpAddress;
    private $tillThreeName;
    private $tillThreeIpAddress;
    private $tillFourName;
    private $tillFourIpAddress;
    private $tillFiveName;
    private $tillFiveIpAddress;

    public function __construct($spreadsheetData, String $storeStatus)
    {
        $this->spreadsheetData = $spreadsheetData;
        $this->setStoreProperties();
    }

    private function setStoreProperties()
    {
        $this->tillModel = $this->spreadsheetData[17];
        $this->tillVersion = $this->spreadsheetData[7];
        $this->tillOneName = $this->spreadsheetData[40];
        $this->tillOneIpAddress = $this->spreadsheetData[41];
        $this->tillTwoName = $this->spreadsheetData[42];
        $this->tillTwoIpAddress = $this->spreadsheetData[43];
        $this->tillThreeName = $this->spreadsheetData[44];
        $this->tillThreeIpAddress = $this->spreadsheetData[45];
        $this->tillFourName = $this->spreadsheetData[46];
        $this->tillFourIpAddress = $this->spreadsheetData[47];
        $this->tillFiveName = $this->spreadsheetData[48];
        $this->tillFiveIpAddress = $this->spreadsheetData[49];
    }

    public function getTills()
    {
        $tillList = [];

        if ($this->tillOneName !== null)
        {
            $tillList[] = [
                'model' => $this->tillModel,
                'name' => $this->tillOneName,
                'ip_address' => $this->tillOneIpAddress,
                'version' => $this->tillVersion,
            ];
        }
        if ($this->tillTwoName !== null)
        {
            $tillList[] = [
                'model' => $this->tillModel,
                'name' => $this->tillTwoName,
                'ip_address' => $this->tillTwoIpAddress,
                'version' => $this->tillVersion,
            ];
        }
        if ($this->tillThreeName !== null)
        {
            $tillList[] = [
                'model' => $this->tillModel,
                'name' => $this->tillThreeName,
                'ip_address' => $this->tillThreeIpAddress,
                'version' => $this->tillVersion,
            ];
        }
        if ($this->tillFourName !== null)
        {
            $tillList[] = [
                'model' => $this->tillModel,
                'name' => $this->tillFourName,
                'ip_address' => $this->tillFourIpAddress,
                'version' => $this->tillVersion,
            ];
        }
        if ($this->tillFiveName !== null)
        {
            $tillList[] = [
                'model' => $this->tillModel,
                'name' => $this->tillFiveName,
                'ip_address' => $this->tillFiveIpAddress,
                'version' => $this->tillVersion,
            ];
        }
        return $tillList;
    }
}

For info, this is used to populate an eloquent model on a controller:

           foreach($storeData->getTills() as $till)
            {
                $store->tills()->create($till);
            }

Answer 1

You might want to create a service that accepts the spreadsheet data as method argument instead.

And maybe the columns could be configured to make it more flexible.

Also no need to do 5 times the same again, instead use a loop.

class SpreadsheetTillsConverter
{
  private int $modelColumn;
  private int $versionColumn;

  /** @var array<int, int> Map nameColumn => ipAddressColumn */
  private array $columns;

  public function __construct(int $modelColumn, int $versionColumn, array $columns)
  {
    $this->modelColumn = $modelColumn;
    $this->versionColumn = $versionColumn;
    $this->columns = $columns;
  }

  public function getTills(array $data): array
  {
     if (!isset($data[$this->modelColumn])) {
        throw new \InvalidArgumentException('Model column not found.');
     }
     $model = $data[$this->modelColumn];

     if (!isset($data[$this->versionColumn])) {
        throw new \InvalidArgumentException('Version column not found.');
     }
     $version = $data[$this->versionColumn];

     $result = [];
     foreach ($this->columns as $nameColumn => $ipAddressColumn) {
        if (isset($data[$nameColumn], $data[$ipAddressColumn])) {
          $result[] = [
            'model' => $model,
            'name' => $data[$nameColumn],
            'ip_address' => $data[$ipAddressColumn],
            'version' => $version,
          ];
        }
     }
     return $result;
  }
}

$converter = new SpreadsheetTillsConverter(17, 7, [
  40 => 41,
  42 => 43,
  44 => 45,
  46 => 47,
  48 => 49
]);

$tills = $converter->getTills($spreadsheetData);