1

I was reading these pages (1,2,3), but I'm still unsure if this violates the guideline.

I have the following data being read from a website:

Date: July 13, 2018 
Type: Partial Solar Eclipse
Location: South in Australia, Pacific, Indian Ocean

Date: July 27, 2018 
Type: Total Lunar Eclipse
Location: Much of Europe, Much of Asia, Australia, Africa, South in North America

An object is constructed that represents the data, and the client can call methods to do work with the data.

In my object the locations are left as a string, because it's easier to search, for example:

class Eclipse():

    def __init__(self, eclipse_location):
       # left out other variables and validation to keep it clear and concise
        self._eclipse_location = eclipse_location

    def takes_place_in(self, country):
        country_formatted = country.strip().title()
        return country_formatted in self._eclipse_location

This way no matter what other words surround the countries, for example "Much of" or "Parts of" it will return True. If the locations were split in a list I would have to create a string and join the country to create "Much of insert country here" and run a search for that string. Also, depending on which part of the site is scraped, the phrase "Much of" isn't always used, sometimes it gets more specific, for example, South in Australia, which means my method has to be changed to accommodate south (and most likely north,east,west). I know because I wrote the method both ways, and keeping it as a string is easier and I still get the behavior I want.

Suppose if I want to create a list of locations, would calling this method violate the guideline that a constructor shouldn't do work?

class Eclipse():

    def __init__(self, eclipse_location):
       # left out other variables and validation to keep it clear and concise
        self._eclipse_location = eclipse_location
        self._locations_list = self._locations_as_list(self._eclipse_location)

    def _locations_as_list(self, str_to_convert):
        return str_to_convert.split(",")

After the object is created, the list doesn't change, and no methods exist to modify (add or remove) the list. To me anyways, it makes sense to create the list once in the constructor, and call the list when ever I need it. If I don't create the list in the constructor anytime I need a list of locations I would have to call _locations_as_list(self, str_to_convert):. This seems inefficient, for a small list it maybe fine, but what if that list contained 100+ elements?

  • 3
    It's not about how long it takes to construct the object, or how long the list is, if the object requires the list in order to function. It's more about whether parsing data should be a responsibility of this particular object, or whether that responsibility should be delegated to some other object. I would argue that parsing is a separate responsibility, demanding a separate class. I don't know how this works out in Python, but in other languages like C# or Java your class's constructor would be handed a fully hydrated list, not some data to be parsed. – Robert Harvey Jun 04 '18 at 21:55
  • Building a list isn’t too bad. Parsing input totally is though. – Telastyn Jun 04 '18 at 23:58
  • Note that a more literal reading of the title (creating a list = instantiating an empty list) renders a different result. Instantiating an empty list is fine. Executing logic that populates the list is not. – Flater Jun 05 '18 at 06:54
  • @RobertHarvey - In your opinion, does putting logic to create and populate the `list` the way I'm suggesting, violate SRP? –  Jun 05 '18 at 18:08
  • I would argue that your class should probably be handed the completed list in the constructor, as I explained in my first comment. – Robert Harvey Jun 05 '18 at 19:56

1 Answers1

2

Yes. It would be bad practice to put your string to list logic in the constructor.

There's plenty of things that can go wrong parsing that string and you have little opportunity to deal with errors when the logic is in the constructor.

Consider what it would be like if you made class which parsed location strings to list.

I would include the following functions:

class LocationParser():
    def IsValid(str_to_convert) #return true if the string can be parsed.

    def Validate(str_to_convert) #return a list of validation errors such as :

        #"illegal character found at pos x", 
        #"unknown country!", 
        #"duplicate country"

    def Parse(str_to_convert) #return the list of locations

Both functions might throw exceptions such as for null or very long strings

now you can create a second version of the object to deal with different languages, or expand the functions so, for example, you can pass in a list of delimiters to use rather than comma.

    def Parse(str_to_convert, delimiters) #return the list of locations

If you tried to put all that logic into your constructor you would face a number of problems, how to return the validation errors or pass in the set the delimiters, how to change the logic out for a differently formatted string etc

Using a separate object or a even a method on the Eclipse object allows you to override the behaviour either through inheritance or composition, or by simply keeping it separate.

Ewan
  • 70,664
  • 5
  • 76
  • 161
  • Splitting a string doesn't seem to have all that many opportunities to fail; the only one I see is OOM... – Deduplicator Jun 04 '18 at 21:40
  • Far better to fail fast in the constructor than carry around bad data waiting to explode when you least expect it. As for all the "what ifs?" YAGNI. If you do, that belongs in a separate class... – user949300 Jun 04 '18 at 21:55
  • 1
    sure, if its just splitting a string. But you already have 'parts of' and 'most of', next there will be a location with a comma in it etc, nulls etc. The best design is clear, whether its worth the time depends on a whole tonne of stuff – Ewan Jun 04 '18 at 21:59
  • 2
    "YAGNI" the cry of the optimist. It's next to no effort to put it its own class, and the times you do need it, it saves your ass – Ewan Jun 04 '18 at 22:03
  • 1
    This would be a better answer if it didn't read like a Monty Python's Flying Circus episode (cue "Ministry of Silly Walks" skit). – Robert Harvey Jun 04 '18 at 22:37
  • hmm,it does look on the the big screen... – Ewan Jun 04 '18 at 22:38
  • @Ewan - Would it be better if `LocationParser` were `static`? If not, you would have the pass a `LocationParser` object to the `Eclipse` `constructor` and then call `lp.Parse(self._eclipse_location)`, correct? –  Jun 05 '18 at 00:15
  • 2
    no, A. that would be a global variable and B. static is always bad. You could inject the parser and call a method yes, but it would be simpler just to parse the string externally and pass the resulting list into the Eclipse constructor. There are several different ways of doing it, choose the one the best fits the style of your code – Ewan Jun 05 '18 at 00:30
  • I'm a bit skeptical of YAGNI myself, but, in your example, you've added two methods for validating the input, with zero input from the imaginary Stack Overflow Stakeholders. :-) I"d argue that **at least** one of them is redundant, and possibly both: why can't `Parse()` just throw an Exception? – user949300 Jun 05 '18 at 00:36
  • @user949300 - I would keep `isValid()` and `Parse()` and depending how big each method gets, it maybe ideal to split into smaller methods. –  Jun 05 '18 at 00:40
  • depends on your use case. sometimes you need a list of all the problems to tell the user – Ewan Jun 05 '18 at 07:50
  • @Ewan - Would you agree that it violates SRP to but the `string` to `list` functions in the `Eclipse` class? After all this class just holds the data to be queried, it's not supposed to create the data and then hold it. Another thing, an Eclipse doesn't create its locations, it just happens and can be seen in various locations. –  Jun 05 '18 at 17:34
  • @Sveta I think you could argue about specific cases, but its not in the same league as 'logic in constructor' – Ewan Jun 05 '18 at 18:00