Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Method to share fields between types #7442

Closed
abeschneider opened this issue Jun 28, 2014 · 11 comments
Closed

Method to share fields between types #7442

abeschneider opened this issue Jun 28, 2014 · 11 comments

Comments

@abeschneider
Copy link

In a real-life example that has been simplified, I have the design:

abstract Player

type PlayerTypeA <: Player
  position::Array{Float64, 1}
  rotation::Float64
end

type PlayerTypeB <: Player
  position::Array{Float64, 1}
  rotation::Float64
end

# ...

type Environment
  players::Array{Player, 1}
end

function update!(player::PlayerTypeA)
  # ...
end

function update!(player::PlayerTypeB)
  # ...
end

function update!(env::Environment)
  for player in env.players
    update!(player)
  end
end

Unfortunately this code violates DRY and becomes difficult to maintain as I add more fields (and there are many more). One way to fix this is by creating a type to hold states:

type PlayerState
  position::Array{Float64, 1}
  rotation::Float64
end

abstract Player
type PlayerA <: Player
  state::PlayerState
end

type PlayerB <: Player
  state::PlayerState
end

However, this creates a problem that I have a tight-coupling with Player and PlayerState. Specifically, I need to know that every Player has a field called 'state' and mandate that other people do the same thing. If this is violated, the error will not be obvious as to why.

I can get around this problem by doing:

state(player::Player) = player.state
position(player::Player) = state(player).position
setposition(player::Player, pos::Array{Float64, 1}) = copy!(state(player).position, pos)
rotation(player::Player) = state(player).rotation
setrotation(player::Player, rot::Float64) = state(player).rotation = rot

So I now have a well-defined interface that solves the previous problem, but also introduces another issue: writing this interface is cumbersome. With a decent number of fields (>> 2), the code quickly gets ugly.

I know this may be considered a repeat of what other people have written for other issues and that there exists multiple proposals for fixing these type of problems (e.g. including fields in abstract types or overloading the '.' operator). However, I thought it might be more productive to put the general problem down without suggesting a specific solution (since those exist already).

@tknopp
Copy link
Contributor

tknopp commented Jun 28, 2014

The discussion started here: https://groups.google.com/forum/?fromgroups=#!topic/julia-users/c17XiHNzyHc

Very related are #4935 and #1974

@tknopp
Copy link
Contributor

tknopp commented Jun 28, 2014

And @pbazant comment in #4935 is spot on: The following is worth reading http://ptgmedia.pearsoncmg.com/images/020163371x/items/item33.html

@abeschneider
Copy link
Author

I read the link you provided (admittedly, I haven't had time to read the entire piece), but I'm not sure I understand the starting premise. I don't see the point of creating an assignment operator for the sub-types. It seems more like an argument for not going with that design rather than not having fields in abstract types.

@tknopp
Copy link
Contributor

tknopp commented Jun 28, 2014

Could you please make a concrete proposal of what you would like to see in Julia? #4935 and #1974 are very different and there have been various concerns that have been raised in these issues.

In your proposal you say that "writing the interface is cumbersome". But even with abstract types with fields you would have to write the interface. Or is your idea to use fields as interface?

#3292 is a proposal to delegate methods from fields to composite types that would make this part a little less type intensive.

@abeschneider
Copy link
Author

I liked the proposal of having abstract classes with fields. The biggest issue people seemed to have with it (e.g. in the thread and by @StefanKarpinski later) was how to deal with constructors.

Other languages (e.g. Scala) have solved this problem by disallowing the abstract type to have constructors. I know this was shot down before because the child class will be affected by changes to the abstract class. However, if you think about the abstract types as interfaces, this should be the case. Additionally, you already have that problem with the grouping method. Thus, abstract types with fields doesn't add any new problems, but it does solve a few.

I'd also love to being able to subclass from multiple abstract types. I know this is being called multiple inheritance, but I think it's closer to Scala's mixins than c++ or Python's MI. I know there are some unresolved issues to the multiple parents, so I'm not arguing as strongly for this, though I would really love to see this as a feature.

The code could look like:

abstract Player
  position::Array{Float64, 1}
  rotation::Float64
end

type PlayerA <: Player
  name::String
  PlayerA()
    obj = new("foo")
    super(obj, ::Player, [0.0, 0.0], pi)
    return obj 
end

type PlayerB <: Player
  PlayerB(pos::Array{Float64, 1}, rot::Float64)
    obj = new()
    super(obj, ::Player, pos, rot)
   return obj
  end
end

move!(player::Player, velocity:Array{Float64, 1}, dt::Float64)
  player.position += velocity*dt
end

a = PlayerA()
b = PlayerB([10.0, 10.0], -pi)
move!(a, [0.1, -0.1], 0.1)
move!(b, [0.1, -0.1], 0.1)

Seems like a much cleaner design to me than having to define all the interface methods for the separate state type. Having a super method to fill in the parent's values allows for the possibility of multiple subclassing, is explicit of what is happening, and allows a well-defined interface for an abstract class that can protect against the tight-coupling problem.

The proposal of overloading the '.' operator might also be able to fix the problem, although it's not as clear to me if that will actually cut down on the code written. However, it might lend itself well to letting macros generating the messy code.

@tknopp
Copy link
Contributor

tknopp commented Jun 28, 2014

Ok lets look at your example how this could be written with current julia

abstract Player

type PlayerA <: Player
  position::Array{Float64, 1}
  rotation::Float64
  name::String

  PlayerA() = new([0.0, 0.0], pi, "foo")
end

type PlayerB <: Player
  position::Array{Float64, 1}
  rotation::Float64

  PlayerB(pos::Array{Float64, 1}, rot::Float64) = new(pos, rot)
end

move!(player::Player, velocity:Array{Float64, 1}, dt::Float64)
  player.position += velocity*dt
end

a = PlayerA()
b = PlayerB([10.0, 10.0], -pi)
move!(a, [0.1, -0.1], 0.1)
move!(b, [0.1, -0.1], 0.1)

So there is no need to define any interface methods. The key thing here is that the move method has not changed. There now is of course an implicit assumption that any Player has a position field.

So comparing your method with mine, the only negative thing is that the fields are repeated. And regarding this I have to agree that abstract types with fields are nice. But IMHO this is not a major thing that is solved here. Its just field repetitions. All the methods which are the more important thing can already be written as if the fields would be there. This is a major difference compared to Java and I think repeating methods or introducing unnecessary boilerplate code for internal accessors is what makes this annoying.

Regarding the super constructor: You can also already have this. consider the following:

abstract Player

function super(player::Player, pos::Array{Float64, 1}, rot::Float64) 
  player.position = pos
  player. rotation = rot
  player
end

type PlayerA <: Player
  name::String
  position::Array{Float64, 1}
  rotation::Float64

  PlayerA() = super(new("foo"),pos,rot)
end

type PlayerB <: Player
  position::Array{Float64, 1}
  rotation::Float64

  PlayerB(pos::Array{Float64, 1}, rot::Float64) = super(new(), pos, rot)
end

But again, I am not really against abstract types with field. Its just that there are more pressing issues in my opinion: abstract multiple inheritance #5 and formal interface definitions #6975.

@JeffBezanson
Copy link
Member

Closing as dup of various existing issues.

@abeschneider
Copy link
Author

@tknopp In this example it doesn't look like too much work to simply copy code, but any code base that gets much larger you will run into maintainability issues. If you have to change a field you might have a lot of subclasses to update (which may be strewn across many classes).

As for the priority, I guess that's a subjective and dependent on what you're doing. I also think multiple inheritance is important. It's just that this issue is affecting my code right now. Also, I suspect both issues will affect each other.

As for super, yes, but my point was simply that was a simple means of dealing with the constructor problem that people bring up when talking about abstract types with fields.

@abeschneider
Copy link
Author

@JeffBezanson I agree that this issue is covered in other places, but I think those issues provide a specific solution. The thought was to keep an issue that was non-implementation specific (i.e. the point is some method to deal with this design decision whether its by abstract types with fields or overload the . operator).

If it's decided that this isn't something Julia should do, then it makes sense to close it.

I won't argue the point further -- I really like Julia and am trying to use it for my various projects. Whether it's something you guys decide is appropriate for the language is another matter. But I thought it was worth voicing my concern as well as others I know who are contemplating using Julia.

@tknopp
Copy link
Contributor

tknopp commented Jun 29, 2014

@abeschneider Yes of course priority is subjective (as this whole issue). The point I wanted to make is that for MI there is no real workaround and projects like Gtk.jl really need this. Java interfaces do support this and without it one is really limited in various situations.

Abstract types with fields can be relatively easily emulated and your last concern about the non-locality of the fields can be worked around by using a macro that contains the field declarations. But again, I am not against it. My vote is for disallowing field overloads and allowing abstract types with fields.

Please note that closing this issue does not mean that it is not a good idea. It just that #4935 is the "official" issue for the thing you want to see and it is easier for all if the discussions are not spread across various issues.

@abeschneider
Copy link
Author

@tknopp That's fair. I've been reading 'you can already do that' as not being important and probably not making it into the language. I am mostly trying to determine if there feature will eventually be included in the future and am fine repeating code for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants