4

This is what the Effective GO had to say about Embedding in golang

When we embed a type, the methods of that type become methods of the outer type, but when they are invoked the receiver of the method is the inner type, not the outer one

I had a code snippet where I had Struct User defined as follows

type User struct {
    Name     string
    Password string
    *sql.Tx
}

And then I call u.Query("some query here") etc. I had done this specifically so that I could avoid calls like u.Transaction.Query("query"), which clearly violates Law of Demeter. Now after reading the docs and effective go, I am doubtful about merits of the first approach as well. Am I violating Law of Demeter? If yes the how can I avoid it ?

icza
  • 389,944
  • 63
  • 907
  • 827
Anony-mouse
  • 2,041
  • 2
  • 11
  • 23
  • 3
    In that code, I'd be more worried about [SoC](https://en.wikipedia.org/wiki/Separation_of_concerns). Does an SQL transaction have anything to do with a user? – Ainar-G Oct 20 '15 at 09:28
  • @Ainar-G User wants to query the database. Then would it be violation of SoC ? – Anony-mouse Oct 20 '15 at 09:34
  • Moreover, even if I pull out the transaction and replace it with another object(say Superpower), I still believe it violates Law of Demeter – Anony-mouse Oct 20 '15 at 09:39
  • Can any of downvoters explain what the issue with the question is ? – Anony-mouse Oct 23 '15 at 08:10

1 Answers1

6

The embedding concept somewhat violates Law of Demeter as it doesn't hide the fact that a type was embedded if the type itself is exported. Note that embedding an unexported type does not violate LoD (you can't refer to unexported fields and methods).

But this doesn't force you to refer to promoted fields or methods in a way that also violates LoD. Embedding itself is just a technique so that you can "outsource" common, shared code to other types; or from another point of view to make use of other types when creating new ones. The way you refer to the promoted fields or methods of the embedded types is what may violate the law.

As you said, if you call it as u.Tx.Query(), that is a clear violation of Law of Demeter: you are using the implementation detail that User embeds *sql.Tx.

But if you call it like this: u.Query() that is ok. This form does not expose or take advantage of the fact that *sql.Tx is embedded. This form will continue to work if implementation changes and *sql.Tx will not be embedded anymore (e.g. it is changed to be a "regular" field or removed completely, and a User.Query() method is added).

If you don't want to allow access to the field value of the exported embedded type, make it an unexported regular field and add an "explicit" User.Query() method which may delegate to the field, e.g.:

type User struct {
    Name     string
    Password string
    tx       *sql.Tx // regular field, not embedded; and not exported
}

func (u *User) Query(query string, args ...interface{}) (*sql.Rows, error) {
    return u.tx.Query(query, args...)
}

Further notes:

In the example, if u.Query() is used, the clients using this are not affected if internals of User are changed (it doesn't matter if u.Query() denotes a promoted method or it denotes a method of User, that is: User.Query()).

If sql.Tx changes, yes, u.Query() might not be valid anymore. But an incompatible sql.Tx is unlikely to happen. And if you're the developer of the changed package, and making incompatible changes, it is your responsibility to change other code that depends on your incompatible change. Doing so (properly updating u.Query()) the client who calls u.Query() will not be affected, the client still doesn't need to know something changed under the hood.

This is exactly what LoD guarantees: if you use u.Query() instead of u.Tx.Query(), if the User changes internally, the client calling u.Query() does not need to know or worry about that. LoD is not a bad thing. You should not drop it. You may choose what principles you follow, but you should also think and not follow everything a chosen principle dictates all the time at any costs.

One more thing to clear: LoD does not involve API incompatible changes. What it offers is if followed, internal changes of an entity will not have effect on other entities using the "public" face of the entity. If sql.Tx is changed in a drastic way that Tx.Query() will not be available anymore, that is not "covered" by LoD.

icza
  • 389,944
  • 63
  • 907
  • 827
  • I believe LOD was in existence to make sure that the changes in one class are propagated to other in a very limited way. Both the approaches you mention, do not seem to solve the problem.(I might be totally wrong but that's how I feel, as delegation to a new method still remains a code smell) – Anony-mouse Oct 20 '15 at 11:24
  • @Anony-mouse In the example, if `u.Query()` is used, the clients using this are not affected if internals of `User` are changed (it doesn't matter if `u.Query()` denotes a promoted method or it denotes a method of `User`, that is: `User.Query()`). – icza Oct 20 '15 at 11:44
  • What about the case where `tx` itself changes? Now `u.Query()` might not be a valid operation at all. Since I have introduced this dependency, any issue with `tx` will directly influence my `User` which should be avoided right? One more thing is LoD such a bad thing? Should I dump it altogether? – Anony-mouse Oct 20 '15 at 11:49
  • @Anony-mouse If `sql.Tx` changes, yes, `u.Query()` might not be valid anymore. But an incompatible `sql.Tx` is unlikely to happen. And if you're the developer of the changed package, and making incompatible changes, it is your responsibility to change other code that depends on your incompatible change. Doing so (properly updating `u.Query()`) the client who calls `u.Query()` will not be affected, the client still doesn't need to know something changed under the hood. – icza Oct 20 '15 at 12:05
  • @Anony-mouse This is exactly what LoD guarantees: if you use `u.Query()` instead of `u.Tx.Query()`, if the `User` changes internally, the client calling `u.Query()` does not need to know or worry about that. LoD is not a bad thing. You should not drop it. You may choose what principles you follow, but you should also think and not follow everything a chosen principle dictates all the time at any costs. – icza Oct 20 '15 at 12:07
  • @Anony-mouse One more thing to clear: LoD does not involve API incompatible changes. What it offers is if followed, internal changes of an entity will not have effect on other entities using the "public" face of the entity. If `sql.Tx` is changed in a drastic way that `Tx.Query()` will not be available anymore, that is not "covered" by LoD. – icza Oct 20 '15 at 12:22
  • Soory I was busy, I think with this comment you have pretty much answered the question. I am closing it now. – Anony-mouse Oct 23 '15 at 08:07
  • can you edit the answer above and add the details to it. It might be better for future references. – Anony-mouse Oct 23 '15 at 08:12
  • @Anony-mouse Done. Thanks for the suggestion. – icza Oct 23 '15 at 08:25