Dilemma of static class containg only static methods

Question

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?

Care to tell us which language you're using? Please tag the question appropriately. — Kerrek SB, Oct 10 '11 at 10:47
Sounds like there's disquiet in the [Kingdom Of Nouns](http://steve-yegge.blogspot.com/2006/03/execution-in-kingdom-of-nouns.html) — spraff, Oct 10 '11 at 10:49
@KerrekSB Its just the class design I wanted to discuss; so the language did not seem important to me. I added the C# tag anyways. — bhootjb, Oct 10 '11 at 10:56
@HauntedGhost: Some languages (real languages?) don't *have* a notion of "static" classes, so I figure that's important. I'd say you're essentially just describing some glorified namespaces. — Kerrek SB, Oct 10 '11 at 10:57
The posted code is fine, it is task oriented procedural code that gets the job done. You picked excellent names, it is easy to follow what happens. You actually do have instance objects, Hotel and MappedHotel. Nothing needs fixing. — Hans Passant, Oct 10 '11 at 11:39
@spraff : Well..yeah! I kind of spit on the Kingdom of Nouns in my work :P But, like the poster, I did not find it necessary to create a class for every verb. Doesnt my design look simple enough? — bhootjb, Oct 10 '11 at 12:28
@KerrekSB : I dont get what you are implying. Could you be more elaborate? — bhootjb, Oct 10 '11 at 12:30
@HauntedGhost: This depends on the language... but a class that has only static members isn't really much of a class. Rather, it's just a naming device to group a couple of related functions together. In C++ you could use a *namespace* for those functions rather than a class. In Java, there are no explicit namespaces, so you use classes and static nested classes. I'm not sure about C#. — Kerrek SB, Oct 10 '11 at 12:34
@KerrekSB: ok. C# follows the same approach as Java in this matter. So considering that in mind, do you have any better approach to this? — bhootjb, Oct 10 '11 at 12:45
@HauntedGhost: I suppose you could reconsider the entire design. Perhaps a proper, stateful class that encapsulates the data file would be a bit more "OO" than just a bunch of loose functions that have to pass state information around. — Kerrek SB, Oct 10 '11 at 13:09
These's no reason pure OOP is necessarily better. Hotels and Addresses are clearly objects. CatalogMapper? Not so much. @HauntedGhost, how is a MappedHotel different to a Hotel? — spraff, Oct 11 '11 at 07:43
@spraff : yeah, I wrote these classes on that basis; I did not find OO necessary here. If C# had allowed standalone functions, I would have happily written that. And MappedHotel consists of information of two hotels that got mapped from two catalogs. So it has different/more data than Hotel. — bhootjb, Oct 11 '11 at 08:00
@spraff : I think you got confused by the name (ah! These nouns!); it should have been more like HotelMapping. It is : `class MappedHotel { string masterHotelId; string targetHotelId; string masterHotelName; string targetHotelName; /* etc */ }` — bhootjb, Oct 11 '11 at 08:34
Would I be right in guessing that Hotel is `{string id, string name, etc}` in which case `MappedHotel` is `{Hotel master, Hotel target}` — spraff, Oct 11 '11 at 09:44

score 1 · Answer 1 · answered Oct 10 '11 at 11:24

1

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.

answered Oct 10 '11 at 11:24

apokryfos

38,771
9
70
114

spraff · Answer 2 · 2011-10-11T10:26:19.010

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.

I agree. I had put GeoCode method there because that task was relevant only to mapping. Still, it seems out of place; I will relocate it at some more relevant/new place. — bhootjb, Oct 11 '11 at 08:01

Dilemma of static class containg only static methods

2 Answers2