Jump to content
Sign in to follow this  
NotionCommotion

Collection classes

Recommended Posts

For some applications, I have been using a collection class instead of an array and it seems to be working out.  I am undecided, however, what should be left to this class versus left to another class.  Should a collection just be the collection, or can/should it have different properties such as a name, ID, etc?

For example, say the class is FileDirectory and is a collection of all the files and directories.  Currently, I am wrapping each of these in FileEntry class which has a name property, ID, etc as well as a method to determine what was changed with those properties since the objects creation.  Maybe unnecessary and I should just move these properties to the collection?

Share this post


Link to post
Share on other sites
8 hours ago, requinix said:

No, I don't think so.  I have a class who's sole purpose if to hold and manage these collections, and using your referenced blog example, the name, score, and health all use the same iterator which I can access using foreach() as I am using the IteratorAggregate interface along with the following.

    public function getIterator() {
        return new \ArrayIterator($this->container);
    }

My question is whether it would be bad to add more functionality to the class.  Right now, all my methods only deal with managing this list of items.  This list of items also has a name and a couple other properties.  Do I allow the name of properties to be changed by this class?  Or do I do what I am currently doing and have a class with the name and other properties and inject the nameless collection into it?  If I leave it as I have it, I need a getter to retrieve the list, but then again I am trying to abide by the S in solid.

Share this post


Link to post
Share on other sites

Unless you need to do certain operations frequently, or some things are rather complicated, then I would keep the collection simple. It's about responsibility, as in whether you think it's worth adding the responsibility of understanding how the various classes work to the collection in order to get whatever you're thinking of getting out of it.

  • Like 1

Share this post


Link to post
Share on other sites
8 hours ago, requinix said:

Unless you need to do certain operations frequently, or some things are rather complicated, then I would keep the collection simple. It's about responsibility, as in whether you think it's worth adding the responsibility of understanding how the various classes work to the collection in order to get whatever you're thinking of getting out of it.

This single responsibility objective is definitely grey  and I think the only way to grasp is by doing.  I've heard people claim that classes should only do one thing, but going too far will result with every class with a single method.  In addition to what you nicely stated, I've read it is also about people which rings some truth.

Share this post


Link to post
Share on other sites
Posted (edited)
On 6/14/2018 at 2:30 PM, NotionCommotion said:

I've heard people claim that classes should only do one thing, but going too far will result with every class with a single method.

And that's bad? Why? Ever seen classes implement nothing more than __invoke? Or a Command with only an execute() method.

What about 2 methods? How many methods should in your opinion a class have?

Edited by ignace

Share this post


Link to post
Share on other sites
1 hour ago, ignace said:

And that's bad? Why? Ever seen classes implement nothing more than __invoke? Or a Command with only an execute() method.

What about 2 methods? How many methods should in your opinion a class have?

In my opinion, a class should have no more and no less than the proper amount of methods for that given class 😎

I actually think a class should not contain methods to implement individual business rules or department needs.  There shouldn't be multiple external events which could cause the class to need to change.  It might be a class with only a single __invoke or it might be many methods.  That being said, my past classes almost always contained methods which did meet meet this criteria and based on your earlier advice about SOILD, I am actively trying to do better.

Share this post


Link to post
Share on other sites

I actually think a class should not contain mix methods to implement individual business rules or department needs.

Share this post


Link to post
Share on other sites
19 minutes ago, NotionCommotion said:

I actually think a class should not contain mix methods to implement individual business rules or department needs.

What do you mean by that? Basically everything written boils down to implementing some rule or need, even if it seems otherwise obvious or trivial.

Classes doing one "thing" is really vague, yeah, so a slightly less vague way of putting it is being responsible for one concept. A collection of files is a concept. A file with its properties is a concept. If there was a need to... I don't know, I never did quite understand your original question... a need to do something involving a collection of files, then the class that represents a concept of a collection of files would be a suitable place for it. On the other hand, a need to do something merely involving one or more files might not belong with the collection class because the need isn't about the collection concept per se (as the collection likely represents a specific concept, say a directory or some search results) but more about literally doing something to multiple files.

Share this post


Link to post
Share on other sites
Posted (edited)
14 hours ago, requinix said:

What do you mean by that? Basically everything written boils down to implementing some rule or need, even if it seems otherwise obvious or trivial.

I mean one shouldn't put methods in a class to implement different business rules.

14 hours ago, requinix said:

I don't know, I never did quite understand your original question... a need to do something involving a collection of files, then the class that represents a concept of a collection of files would be a suitable place for it. On the other hand, a need to do something merely involving one or more files might not belong with the collection class because the need isn't about the collection concept per se (as the collection likely represents a specific concept, say a directory or some search results) but more about literally doing something to multiple files.

Still messing around with the chart thing (https://forums.phpfreaks.com/topic/307291-using-a-service-based-on-id-of-record-requested/).  I have a BarChart object and a PieChart object and each will have various methods to manage them.  I create them using the service and mapper PHP objects at the bottom.  Each of these have collections of series and categories which I am referring to as "seriesNodes" and "categoryNodes".  For some, these nodes include collections of their own.

My question was whether a node with a collection should be replaced with a NodeWithCollection object which includes both the name, id, etc property plus the collection array (similar to https://forums.phpfreaks.com/topic/307366-determine-changes-to-an-object/?do=findComment&comment=1558900) or whether I should keep it as is and inject a generic collection in a node.  With little earlier information, you recommended that later and I tend to agree.

As long as I posted all the background information and script, are you able to tell me if "big picture" I am going about this right?

Thanks

PS.  My PHP still shows me sneaking an object into an array to get by the constructor complaining, but I will be fixing it.

 

{
    "chart": {
        "type": "bar"
    },
    "categories": [
        {
            "id": 107,
            "name": "Africa",
            "position": 1,
            "metadata": "bla"
        },
        {
            "id": 117,
            "name": "America",
            "position": 2,
            "metadata": "bla"
        },
        {
            "id": 137,
            "name": "Asia",
            "position": 3,
            "metadata": "bla"
        }
    ],
    "series": [
        {
            "id": 5,
            "name": "Year 1800",
            "position": 1,
            "data": [
                {
                    "id": 107,
                    "name": "1800 Africa",
                    "metadata": "bla"
                },
                {
                    "id": 108,
                    "name": "1800 America",
                    "metadata": "bla"
                },
                {
                    "id": 112,
                    "name": "1800 Asia",
                    "metadata": "bla"
                }
            ]
        },
        {
            "id": 7,
            "name": "Year 1900",
            "position": 2,
            "data": [
                {
                    "id": 137,
                    "name": "1900 Africa",
                    "metadata": "bla"
                },
                {
                    "id": 158,
                    "name": "1900 America",
                    "metadata": "bla"
                },
                {
                    "id": 172,
                    "name": "1900 Asia",
                    "metadata": "bla"
                }
            ]
        },
        {
            "id": 8,
            "name": "Year 2000",
            "position": 3,
            "data": [
                {
                    "id": 117,
                    "name": "2000 Africa",
                    "metadata": "bla"
                },
                {
                    "id": 128,
                    "name": "2000 America",
                    "metadata": "bla"
                },
                {
                    "id": 132,
                    "name": "2000 Asia",
                    "metadata": "bla"
                }
            ]
        }
    ]
}
{
	"chart": {
		"type": "pie"
	},
	"series": [
		{
			"id": 5,
			"name": "Continent",
			"position": 1,
			"metadata": "bla",
			"data": [
				{
					"id": 107,
					"name": "Africa",
					"position": 1,
					"metadata": "bla"
				},
				{
					"id": 117,
					"name": "America",
					"position": 2,
					"metadata": "bla"
				},
				{
					"id": 137,
					"name": "Asia",
					"position": 3,
					"metadata": "bla"
				}
			]
		},
		{
			"id": 5,
			"name": "Continent by year",
			"position": 1,
			"metadata": "bla",
			"data": [
				{
					"id": 107,
					"name": "Africa 1800",
					"position": 1,
					"metadata": "bla"
				},
				{
					"id": 111,
					"name": "America 1800",
					"position": 2,
					"metadata": "bla"
				},
				{
					"id": 123,
					"name": "Asia 1800",
					"position": 3,
					"metadata": "bla"
				},
				{
					"id": 133,
					"name": "Africa 1900",
					"position": 4,
					"metadata": "bla"
				},
				{
					"id": 134,
					"name": "America 1900",
					"position": 5,
					"metadata": "bla"
				},
				{
					"id": 137,
					"name": "Asia 1900",
					"position": 6,
					"metadata": "bla"
				},
				{
					"id": 144,
					"name": "Africa 2000",
					"position": 7,
					"metadata": "bla"
				},
				{
					"id": 145,
					"name": "America 2000",
					"position": 8,
					"metadata": "bla"
				},
				{
					"id": 155,
					"name": "Asia 2000",
					"position": 9,
					"metadata": "bla"
				}
			]
		}
	]
}
$barChartService = new ChartTypes\BarChart\BarChartService(
    new ChartTypes\BarChart\BarChartMapper(
        new ChartTypes\BarChart\BarSeriesMapper($this->pdo, $this->accountsId, $this->properties['id']??null),
        new ChartTypes\BarChart\BarCategoryMapper($this->pdo, $this->accountsId, $this->properties['id']??null),
        $this->pdo,
        $this->accountsId,
        $this->properties
    ),
    ($this->validationService)('chart/bar', $this->properties),
    $this->pdo
);
$barChartEntity=$barChartService->getMapper()->read();

$pieChartService = new ChartTypes\PieChart\PieChartService(
    new ChartTypes\PieChart\PieChartMapper(
        new ChartTypes\PieChart\PieSeriesMapper(
            new ChartTypes\PieChart\PieCategoryMapper($this->pdo, $this->accountsId, $this->properties['id']??null),
            $this->pdo,
            $this->accountsId,
            $this->properties['id']??null
        ),
        $this->pdo,
        $this->accountsId,
        $this->properties
    ),
    ($this->validationService)('chart/pie', $this->properties),
    $this->pdo
);
$pieChartEntity=$pieChartService->getMapper()->read();
namespace NotionCommotion\Chart\ChartTypes\BarChart;
use \NotionCommotion\Chart\Entities;
class BarChartMapper extends \NotionCommotion\Chart\Mappers\ChartMapper
{
    public function read(){
        $sql='SELECT bla bla bla';
        $stmt=$this->pdo->prepare($sql);
        $stmt->execute([$this->properties['id']]);
        $temp=[];
        $categories=[];
        foreach($stmt as $rs) {
            if(!isset($temp[$rs->id_s])) {
                $temp[$rs->id_s]=['id'=>$rs->id_s, 'name'=>$rs->name_s, 'position'=>$rs->position_s, 'metadata'=>$rs->metadata_s, 'data'=>[]];
            }
            if(!isset($categories[$rs->id_c])) {
                $categories[$rs->id_c]=new Entities\CategoryNode(['id'=>$rs->id_c, 'name'=>$rs->name_c, 'position'=>$rs->position_c, 'metadata'=>$rs->metadata_c]);
            }
            $temp[$rs->id_s]['data'][]=new Entities\PointNode(['id'=>$rs->id_d, 'name'=>$rs->name_d, 'metadata'=>$rs->metadata_d]);
        }
        $categories=array_values($categories);
        $series=[];
        foreach($temp as $node) {
            $node['data']=new Entities\PointsCollection(...$node['data']);
            $series[]=new Entities\SeriesNode($node);
        }
        $chart=new BarChartEntity(
            $this->getBaseProperties(),
            new Entities\SeriesCollection(...$series),
            new Entities\CategoriesCollection(...$categories)
        );
        return $chart;
    }
}
namespace NotionCommotion\Chart\ChartTypes\PieChart;
use \NotionCommotion\Chart\Entities;
class PieChartMapper extends \NotionCommotion\Chart\Mappers\ChartMapper
{
    public function read(){
        $sql='SELECT bla bla bla';
        $stmt->execute([$this->properties['id']]);
        $series=[];
        $categories=[];
        foreach($stmt as $rs) {
            if(!isset($series[$rs->id_s])) {
                $series[$rs->id_s]=['id'=>$rs->id_s, 'name'=>$rs->name_s, 'position'=>$rs->position_s, 'metadata'=>$rs->metadata_s, 'categoryCollection'=>[]];
            }
            if($rs->id_c) {
                $point=new Entities\PointNode(['id'=>$rs->id_p, 'name'=>$rs->name_p, 'metadata'=>$rs->metadata_p]);
                $series[$rs->id_s]['categoryCollection'][]=new Entities\CategoryNode(['id'=>$rs->id_c, 'name'=>$rs->name_c, 'position'=>$rs->position_c, 'metadata'=>$rs->metadata_c], $point);
            }
        }
        $series=array_values($series);
        foreach($series as $key=>$seriesNode) {
            $seriesNode['categoryCollection']=array_values($seriesNode['categoryCollection']);
            $seriesNode['categoryCollection']=new Entities\CategoriesCollection(...$seriesNode['categoryCollection']);
            $series[$key]=new Entities\SeriesNode($seriesNode);
        }
        $chart=new PieChartEntity(
            $this->getBaseProperties(),
            new Entities\SeriesCollection(...$series)
        );
        return $chart;
    }
}

 

Edited by NotionCommotion
Showed how entities are created.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
Sign in to follow this  

×

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.