3

I am making an OO design for an app. So far I've come with this, I looks like it could work, but I am not sure of correctness of its design.

Old design enter image description here

EDIT

Okay so, I used factory pattern, and it looks good to me. Is this good design ? enter image description here

You are building application that will load data from several different advertising systems and then store the data into database, so it will be possible to analyze them.

You need to take in mind that every advertising system has different structure of report: Different named columns, different order of columns, different date formats. Also data from ad systems are in different data formats(JSON,CSV,XML),

Reports from each system contains different amount of columns, our application is interested only in some of them: date, ad_campaing, ad_group, keyword, impressions, price (in every system they have different name)

closed as unclear what you're asking by 8bittree, Thomas Owens Aug 15 '17 at 16:37

Please clarify your specific problem or add additional details to highlight exactly what you need. As it's currently written, it’s hard to tell exactly what you're asking. See the How to Ask page for help clarifying this question. If this question can be reworded to fit the rules in the help center, please edit the question.

  • Could you find a design pattern that is close? You may get more responses if you are able to have a more specific question about a particular pattern. – JeffO Dec 1 '16 at 11:14
  • I am going through them right now and trying to think of which would be good. But I think to find one that will fit, will require to change the whole logic of what I have. – rtom Dec 1 '16 at 11:20
  • So actually you are saying that Bing and Yahoo are AdSystems? – qwerty_so Dec 1 '16 at 11:51
  • Right... The Bing/Adwords/Yahoo classes technically don't need to be there, I only have them there because in reportColumns I saved which reportColumns I am interested in. I could saved it in config file for example. – rtom Dec 1 '16 at 11:58
  • 'looks like it could work', for us to know for sure we would need some requirements. – Kwebble Aug 8 '17 at 19:20
1

Your report data layer is not Open-Closed (https://en.m.wikipedia.org/wiki/Open/closed_principle), it knows all formats and must be changed in order to be extended with new formats.

The design didn't explain how the reports are constructed, which is the heart of this system. This is something you may wish to address at this stage.

0

I would go with

Advert
{
    string id;
    var ad_group;
    var keyword;
    var impressions;
    var price ;
}

AdRepository_MyDatabase
{
    InsertAdvert(Advert advert);
    GetAdverts();
}

//this class is the only thing that knows about the bing format and how to
//get the bing adverts and convert them to our adverts
AdRepository_Bing
{
    List<Advert> GetAdverts();
}

AdRepository_Google
{
    List<Advert> GetAdverts();
}

etc

WorkerService
{
    DownloadAdverts()
    {
         foreach(repo in RepositoryCollection)
         {
              var ads = repo.GetAdverts();
              foreach(ad in ads)
              {
                     mydatabaseRepo.InsertAdvert(ad);
              }
         }
    }
}
  • Yea, except some class for loading the data(downloading/opening files) it's basically same as what I did. And I am missing some class that will glue it together – rtom Dec 2 '16 at 12:04
  • So, you are asking on how to connect the ReportLoader with an AdvertisingFactory? – J. Pichardo Dec 10 '16 at 8:38
0

I do believe it is a good architecture, but I came with a question, how would you say to the AdvertisingFactory how to convert the string to a report, I hope I understood the question right. So it came to me an implementation of an Strategy Pattern:

Strategy Pattern

There the one that does your conversion is the ReportMapper, because I didn't found another name, and the factory wraps it all.

  • I get report data in form of array, and in report class I select only values that I need with selectData method – rtom Dec 11 '16 at 2:26
  • That I didn't specify because you had already done so in the question. – J. Pichardo Dec 11 '16 at 2:34
  • The Mapper exists just to convert the string to report, there you report loader converts it to array. – J. Pichardo Dec 11 '16 at 2:37

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