[depricated]

So I'm reading Clean Code, and there have been several instances that made me cringe as I was soundly mocked by the book. Fair enough, I feel like I'm learning a lot from it. I went through and crunched down a couple abominations I had into clearly readable functions that I'm rather proud of.

Anyway, one thing I'm having a little difficulty with is the way the book is presenting the Law of Demeter. That is, I think I understand, but I want to know I understand. If I'm correct, they way I'm implementing this is 'correct.' If I'm not, then I'm misunderstanding.

Rather than giving you chunks of code with what I've written I'll give a basic description - and if more information will help by all means I'll provide it. Maybe I'm confusing two parts of the chapter, even.

So the Law of Demeter, as presented by the book, states that "a module should not know the innards of the objects it manipulates." It goes on to clarify that:

Robert C. Martin, Clean Code, Page 97-98 said:

a method ƒ of a class C should only call the methods of these:

• C
  
• An object created by ƒ
  
• An object passed as an argument to ƒ
  
• An object held in an instance variable of C

The method should not invoke methods on objects that are returned by any of the allowed functions.

This is in the context of discussing Data Structures as opposed to Objects. I have a data structure, and an object I'm using that handles the data from the data structure, once the data is created.

I have two classes: TemplateData and TemplateMailer


TemplateData consists of a number of private variables, with setters/getters.

name,path,preSubject,postSubject are all set by function SetDBValues(name, tier) which does a DB query to get the data based on the name of the template needed and the customer's service tier.
the rest of the variables (about 15) use setters/getters.

The TemplateMailer is fairly simple. It has two functions. BuildTemplate(templateData) and Send(). It creates a mail item(The mail item is a class variable so once it's created it's held by the mailer. ), loads the HTML from the template into the mailitem, and then replaces strings in the template with information from the TemplateData. Actually, this would probably make more sense if I showed that function.

Set outlookApplication = New Outlook.Application
Set outlookItem = oApplication.CreateItemFromTemplate(data.GetPath)

With outlookItem
    'XXXXX ticket is a field dynamically added to each template. As such we need
    'to do some quick logic to make sure it exists before we add it. So do XXXXX first.
    If Not data.GetXXXXX = "" Then .HTMLBody = Replace(.HTMLBody, "INSERT_XXXXX_LABEL", "XXXXX Ticket#:") Else .HTMLBody = Replace(.HTMLBody, "INSERT_XXXXX_LABEL", "")
    .HTMLBody = Replace(.HTMLBody, "INSERT_XXXXX_TICKET", data.GetXXXXX)
        
    'set date/times
    .HTMLBody = Replace(.HTMLBody, "INSERT_INCIDENT_START_DATE_TIME", Format(data.GetStartTime, "mm/dd/yyyy h:mm AMPM") & " EST")
    .HTMLBody = Replace(.HTMLBody, "INSERT_INCIDENT_END_DATE_TIME", Format(data.GetEndTime, "mm/dd/yyyy h:mm AMPM") & " EST")
    .HTMLBody = Replace(.HTMLBody, "INSERT_UPDATE_DATE_TIME", Format(data.GetNextUpdate, "mm/dd/yyyy h:mm AMPM") & " EST")
    
    'set ticket data
    .HTMLBody = Replace(.HTMLBody, "INSERT_REMEDY_TICKET", data.GetRemedy)
    .HTMLBody = Replace(.HTMLBody, "INSERT_XXXXX_TICKET", data.GetXXXXX)
    .HTMLBody = Replace(.HTMLBody, "INSERT_PRIORITY", data.GetPriority)
    
    'set verbiage
    .HTMLBody = Replace(.HTMLBody, "INSERT_INCIDENT_DESCRIPTION", data.GetShortDescription)
    .HTMLBody = Replace(.HTMLBody, "INSERT_SITUATION", data.GetSituation)
    .HTMLBody = Replace(.HTMLBody, "INSERT_IMPACT", data.GetImpact)
    .HTMLBody = Replace(.HTMLBody, "INSERT_FIX", data.GetFix)
    .HTMLBody = Replace(.HTMLBody, "INSERT_ASSESSMENT_IMPACT", data.GetAssessment)
    
    'Signature and Subject
    .HTMLBody = Replace(.HTMLBody, "INSERT_XXXXX_SIGNATURE", DLookup("strEmpName", "tblCurrentXXXXX"))
    .subject = data.GetPreSub & " " & data.GetShortDescription & data.GetPostSub
    .BCC = .BCC & "; " & data.GetBCC
    .CC = data.GetCC

End With

End Function

So I'm trying to determine: have I successfully separated the data from the object, or is there a better way I should go about this? Does my Mailer know too much about my Data? If I'm understanding the book correctly, then I think I'm doing this right. If not, I would love some clarification.

Better yet, is this a complete mess entirely? I'd love suggestions, corrections, critiques - I want to be a good programmer some day, with pretty and professional looking code. Since the above is already in production it's not likely to be changed any time soon, but I'd still like to be able to do better moving forward.

I truly hope this has made sense.

(aside, the XXXXX in the code section is a client name that had to be chopped out)

This post has been edited by depricated: 01 November 2012 - 11:22 AM

[lordofduct]

Looks fine to me, you break none of the rules supplied.

The only object you modify is an object created by this function (rule 2), using data passed in argument (rule 3).

[depricated]

Then I think I do understand. A violation would be operating by exposing an object through another object? Such as if I were to, say, expose the outlookItem in the Mailer and start running functions directly on the outlookItem from another class that is utilizing the mailer - since the mailer isn't a data structure but an object, so its innards should be hidden.

Do I have that right? I'll be really happy if I do

Thanks for sifting through my mess of a post, by the way, I think my mind was driving in 3 different directions when I wrote that.

This post has been edited by depricated: 02 November 2012 - 07:00 AM

[mojo666]

That is correct. A simplistic example is given here demonstrating what the law of demeter is trying to prevent and why such violations are bad. The example sets up a business, a customer, and transactions are performed by the customer giving the business his wallet to take out the necessary money. Anyone can understand the issues with such a setup and realize it is better for the customers to manage their own wallet and directly exchange cash with the business.

[BobRodes]

That's a nice example, mojo. Depricated, for some related ideas, have a look at "level of indirection", "cohesion" and "coupling".