It's a private init method and the code is right next to the new (probably): just set em - what else is an init for?

Adding a pile of common code to the constructor so it performs half the initialization just seems silly, especially if there are multiple constructors for the object and the init provides the common work.

You're not hiding stuff away (the "action at a distance" concern) because you've said up front that the init routine is going to init stuff - that's what an init routine is for after all. Any maintainer who can't figure out that a routine called init is going to update the object in interesting ways probably is in for a bunch of trouble (or is about to create a bunch of trouble) anyway.

Update: I would call such a function _init(). The leading underscore indicates an internal function.

Argument passing between methods should mainly be for carrying user suplied values to the place where the sanity of them is to be determined, and where the value might be approved.

What everyone else is telling you is essentially that the wall of the object class is the only encapsulation barrier that you need to respect, and when you're on the inside of it anything goes (or almost anything). What I would suggest is that the situation is a little fuzzier than that, and it's not a bad idea to try to organize the way communication happens inside of the object, as well as between objects.

Possibly a related issue: I often find that when I want to make sure that one thing is kept up to date if a change happens to something else, it's very tempting to bury a call inside the "something_else" accessor to some other routine that will refresh what ever needs to be kept up to date. That's a very robust (if not always efficient) way of doing things -- the trouble is when I come back six months later, I tend to forget about it, and assume that all my accessors are simple and stupid accessors. So: nice trick, but it's good to take it easy on it...

To call a method to calculate one attribute is totally ok and something else than a method which calculates all attributes and returns them as a list.

Besides, re "action at a distance", that applies to setting global variables, but that's not the case here, as you alter an object you pass as a read/write parameter.

If you insist on turning init into a function, at least, make it return a hash.

sub my_init {
  my ($self, $args) = @_;
  # do stuff with $args;
  return { attr_1 => $attr_1, attr_2 => $attr_2 };
}

sub new {
  my ($class, $args) = @_;
  my $self = {};
  bless ($self, $class);
  my $attrs = $self->my_init($args);
  foreach (keys %$attrs) {
    $self->{$_} = $attrs->{$_};
  }
  return $self;
}
[download]

I still prefer directly setting the attributes for the object you passed as a parameter. It smells cleaner to me: there's far less distracting "janitor code" in it. (That is code that is necessary for an implementation but actually doesn't do much useful for its perceived complexity; where nothing specifically applies to this particular piece of code. Up to 4 lines of code for a simple "copy attributes" action: ugh.)

For proper advice, some questions need answers. You label my_init() as a private method. Why? Is that method called from some other method (public or private) of the package? In your code examples my_init() is unconditionally called from new(). Why?

As you post them, your new() and my_init() methods could as well be merged into one. Make up your mind on why you need the attribute population separated from the constructor.

There are of course good reasons for doing that.

• an empty object might be needed before the attributes are even known
• there might be a need to re-initialize the attributes of an existing object

For either use, my preferred idiom is

sub new {
    my $class = shift;
    my $self  = bless {}, $class;
    @_ ? $self->init(@_) : $self;
}

sub init {
    my $self = shift;
    # do parameter validation, set attributes
    ...
    # and return the object
    $self;
}
[download]

IMHO attributes should only be passed around using setters/getters, and an init() subroutine is a catch-all setter method to me. It might do other initializing stuff, though.

_($_=" "x(1<<5)."?\n".q·/)Oo.  G°\        /
                              /\_¯/(q    /
----------------------------  \__(m.====·.(_("always off the crowd"))."·
");sub _{s./.($e="'Itrs `mnsgdq Gdbj O`qkdq")=~y/"-y/#-z/;$e.e && print}