CODEWITHSUNDEEP
c#oopclass-designstatic-class
Bhoot
Bhoot

Reputation: 1541

Dilemma of static class containg only static methods

So I have this task of mapping two hotel catalogs; both are csv files. I created two classes, based on their responsibilities : 1. CatalogManager : handles the I/O operations for catalogs. 2. CatalogMapper : Handles the mapping task of two catalogs.

The definitions are as follows :

public static class CatalogManager
{
   public static List<Hotel> GetHotels(string filePath) { }
   public static void SaveHotels (List<Hotel> hotels, string filePath) { }
   public static void SaveMappedHotels (List<MappedHotel> hotels, string filePath) { }
   public static List<string> GetHotelChains(string filePath) { }
}

public static class CatalogMapper
{
   public static List<MappedHotel> MapCatalogs (List<Hotel> masterCatalog, List<Hotel> targetCatalog) { }

   public static FetchAddressGeoCodes (Hotel.Address address)
   { // fetch address's geocode using Google Maps API }

   public static string GetRelevantHotelChain (string hotelName)
   {
      List<string> chains = CatalogManager.GetChains();
      // find and return the chain corresponding to hotelName. 
   }
}

A typical mapping operation may go something like :

List<Hotel> masterCatalog = CatalogManager.GetHotels(masterFilePath);
List<Hotel> targetCatalog = CatalogManager.GetHotels(targetFilePath);
List<MappedHotel> mappedHotels = CatalogMapper.MapHotels(masterCatalog, targetCatalog);
CatalogManager.SaveMappedHotels(mappedHotels, mappedCatalogFilePath);

As the code shows, both the classes are static. Though I found them right and working, I still feel there is something wrong with this design in terms of OOP. Is it fine that both of the classes are simply static? I found no need of instantiating them. Also, what are other flaws in this design? I am sure flaws are present. What are the solutions for those?

Upvotes: 0

Views: 246

Answers (2)

spraff
spraff

Reputation: 33405

Fear not the free function!

I find it suspicious that CatalogMapper maps a Hotel.Address. To be properly factored/encapsulated, either

  • CatalogMapper should operate on a non-Hotel-specific Address,

  • or, if Hotel.Address is somehow special w.r.t GeoCodes then Hotel.Address should be able to map itself to a GeoCode without a CatalogMapper.

With clarifications from the comments, I'd like to propose the following. I don't know C# so I'll write this as C++. I hope it translates

struct Hotel {
    const Address & address () const;

    // I typedef EVERTYTHING :-)    
    typedef std :: list <std :: string> StringList;
    typedef std :: pair <Hotel, Hotel> Pair;
    typedef std :: list <Hotel> Container; // TODO implicit sharing?
    typedef std :: list <Pair> Mapping;

    // NB will be implemented in terms of std::istream operations below,
    // or whatever the C# equivalent is.
    // These could arguably live elsewhere.
    static Container load (const std :: string &);
    static Mapping load_mapping (const std :: string &);
    static void save (const std :: string &, const Container &);
    static void save_mapping (const std :: string &, const Mapping &);

    // No need for a "Manager" class for this.
    static StringList load_chain (const string & file_name);

private:
    static Hotel load (std :: istream &);
    void save (std :: ostream &) const;
};

// Global namespace, OK because of overloading. If there is some corresponding
// generic library function which merges pair-of-list into list-of-pair
// then we should be specialising/overloading that.
Hotel :: Mapping merge (const Hotel :: Container &, const Hotel :: Container &);

struct GeoCode {
   GeoCode (const Address &);
};

Whenever I see a class called a "Manager", if it's not an object which creates, owns, and controls access to other objects, then it's warning alarm that we're in the Kingdom Of Nouns. Objects can manage themselves. You should only have a separate class for the logic if that logic reaches beyond the realm of a single class.

Upvotes: 1

apokryfos
apokryfos

Reputation: 40683

Another reasonable approach would be to make CatalogManager a non-static class initialized with the file-name. This would allow you to use the files partially from memory and read or write when its required.

Upvotes: 1

Related Questions