NotionCommotion Posted June 13, 2018 Share Posted June 13, 2018 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? Quote Link to comment Share on other sites More sharing options...
requinix Posted June 13, 2018 Share Posted June 13, 2018 You mean like having the collection use an array of names, an array of IDs, and an array for each property? Quote Link to comment Share on other sites More sharing options...
NotionCommotion Posted June 14, 2018 Author Share Posted June 14, 2018 8 hours ago, requinix said: You mean like having the collection use an array of names, an array of IDs, and an array for each property? 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. Quote Link to comment Share on other sites More sharing options...
requinix Posted June 14, 2018 Share Posted June 14, 2018 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. 1 Quote Link to comment Share on other sites More sharing options...
NotionCommotion Posted June 14, 2018 Author Share Posted June 14, 2018 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. Quote Link to comment Share on other sites More sharing options...
ignace Posted June 15, 2018 Share Posted June 15, 2018 (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 June 15, 2018 by ignace Quote Link to comment Share on other sites More sharing options...
NotionCommotion Posted June 15, 2018 Author Share Posted June 15, 2018 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. Quote Link to comment Share on other sites More sharing options...
NotionCommotion Posted June 16, 2018 Author Share Posted June 16, 2018 I actually think a class should not contain mix methods to implement individual business rules or department needs. Quote Link to comment Share on other sites More sharing options...
requinix Posted June 16, 2018 Share Posted June 16, 2018 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. Quote Link to comment Share on other sites More sharing options...
NotionCommotion Posted June 16, 2018 Author Share Posted June 16, 2018 (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 June 16, 2018 by NotionCommotion Showed how entities are created. Quote Link to comment Share on other sites More sharing options...
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.