Should I expose an instance's state with methods?

Question

I've read somewhere about a pattern that is as follows: if it'll change the state of the instance, the method should be named with a verb and return void; and if the method just returns something (computed or simply a property), it should be named as a noun. E. g.:

dog.walk() returns void and changes the instance's state. dog.barkVolume() will return a int and changes nothing. Another example is dog.runAfterTail() returns void and changes the state while dog.name() will return and string also and change nothing.

As you can see, the instance's state is exposed through methods instead of getters or simply the property. When I see properties being exposed through a method (named with a noun), it seems like it's returning an immutable value (since I am talking about JavaScript/TS, it would be something like return [...this.array_typed_property]) and when using a plain property or getter, it would return something like return this.property (possible to be mutated with .pop()).

Would you guys agree with that and recommend it (or not)?

I have this impression over methods because they seems more "closed", differently from a property, which seems to be exposing the state of the instance as if I were going into the instance's bowls and taking it from there.

Also, is there a name for that pattern?

This is what I have so far. Things are not very well structured and the readonlys are only enforced on dev mode since I can't Object.freeze some arrays because they're use along with Vue, which can't watch changes on a freezed object.

In these examples, another doubt of mine would be how to properly expose properties that do change in the No class. For example, the property name won't change, ever, so it seems fine to simply add a readonly, but it also would feel safer to freeze it and maybe expose through a getter or method. Or, maybe, expose it with a method like .toDTO() which would return an object of all the non-changing properties of my class, freezed.

import type { TreeviewItem } from '@/app/ui/props.d'
import { No } from '@/app/ui/treeview/no'
export class Tree {
  private readonly _nodes: Array<No>
  private _focused_node: No | null = null
  private _selected_node: No | null = null
public constructor(items?: TreeviewItem) {
    this._nodes = items
      ? items.map(
          (item, i) =>
            new No({ ...item, address: item?.address ?? [i] }, null, this)
        )
      : []
  }
public nodes(): ReadonlyArray<No> {
    return this._nodes
  }
public focusedNode(): No | undefined {
    return this._focused_node ?? undefined
  }
public selectedNode(): No | undefined {
    return this._selected_node ?? undefined
  }
public visibleNodes(): ReadonlyArray<No> {
    const visible_nodes: Array<No> = []
    const plain = (nos: Array<No>) => {
      nos.forEach(no => {
        visible_nodes.push(no)
        plain(no.visibleChildren())
      })
    }
    plain(this._nodes)
    return Object.freeze(visible_nodes)
  }
public focusNode(no: No): void {
    this._focused_node = no
  }
public isChildNodeFocused(address: number[]): boolean {
    if (!this._focused_node) {
      return false
    }
return this._focused_node.addressToString().startsWith(address.join('-'))

}
public goUp(): void {
    const visible_nodes = this.visibleNodes()
    const index = visible_nodes.findIndex(n => n === this._focused_node)
    if (index > 0) {
      this.focusNode(visible_nodes[index - 1])
    }
  }
public goDown(): void {
    const visible_nodes = this.visibleNodes()
    const index = visible_nodes.findIndex(n => n === this._focused_node)
    if (index < visible_nodes.length - 1) {
      this.focusNode(visible_nodes[index + 1])
    }
  }
public openOrFocusFirstChild(): void {
    if (!this._focused_node || this._focused_node.isLeaf()) {
      return
    }
this._focused_node.isOpen()
  ? this.focusNode(this._focused_node.nodes[0])
  : this._focused_node.toggleOpen()

}
public closeOrFocusParent(): void {
    if (!this._focused_node) {
      return
    }
this._focused_node.isOpen()
  ? this._focused_node.toggleOpen()
  : this.focusNode(this._focused_node.parent()!)

}
public goFirstNode(): void {
    const visible_nodes = this.visibleNodes()
    if (visible_nodes.length > 0) {
      this.focusNode(visible_nodes[0])
    }
  }
public goLastNode(): void {
    const visisible_nodes = this.visibleNodes()
    if (visisible_nodes.length > 0) {
      this.focusNode(visisible_nodes[visisible_nodes.length - 1])
    }
  }
public closeAll(): void {
    const fecharNos = (nos: No[]) => {
      nos.forEach(no => {
        if (!no.isLeaf() && no.isOpen()) {
          no.toggleOpen()
        }
        fecharNos(no.nodes)
      })
    }
fecharNos(this._nodes)
this.goFirstNode()

}
public selectNode(no: No): void {
    this._selected_node = no
    this.focusNode(no)
  }
public closeAllLeaves(): ReadonlyArray<No> {
    const leaves: Array<No> = []
const planificar = (nos: Array&lt;No&gt;) =&gt; {
  nos.forEach(no =&gt; {
    if (no.isLeaf()) {
      leaves.push(no)
    }
    planificar(no.nodes)
  })
}
planificar(this._nodes)

return Object.freeze(leaves)

}
public nodeLikeName(nome: string): No | undefined {
    const plain = (nodes: Array<No>): No | undefined => {
      for (const node of nodes) {
        if (node.name.includes(nome)) {
          return node
        }
        const encontrado = plain(node.nodes)
        if (encontrado) {
          return encontrado
        }
      }
      return undefined
    }
return plain(this._nodes)

}
public nodeByAddress(endereco: number[]): No | undefined {
    let node: No | undefined = this._nodes[endereco[0]]
for (let i = 1; i &lt; endereco.length; i++) {
  if (!node) {
    return undefined
  }
  node = node.nodes[endereco[i]]
}

return node

}
public toPlainObject(): TreeviewItem {
    const transformRecursively = (nodes: Array<No>): TreeviewItem => {
      return nodes.map(node => ({
        address: node.address,
        badge: node.badge,
        expanded: node.isOpen(),
        items: transformRecursively(node.nodes),
        loading: node.isLoading(),
        title: node.name,
        value: node.value,
      }))
    }
return transformRecursively(this._nodes)

}
public openNode(endereco: Array<number>): void {
    let node: No | undefined = this._nodes[endereco[0]]
for (let i = 1; i &lt; endereco.length; i++) {
  if (!node) {
    return undefined
  }
  node.toggleOpen()
  node = node.nodes[endereco[i]]
}

}
public openParentNode(): ReadonlyArray<No> {
    const parents: Array<No> = []
const getOpenParents = (nodes: ReadonlyArray&lt;No&gt;) =&gt; {
  for (const node of nodes) {
    if (node.isOpen() &amp;&amp; !node.isLeaf()) {
      parents.push(node)
    }

    if (!node.isLeaf()) {
      getOpenParents(node.nodes)
    }
  }
}

return Object.freeze(parents)

}
}

import type { BadgeProps, TreeviewItem } from '@/app/ui/props.d'
import type { Tree } from '@/app/ui/treeview/tree'

export class No {
  private readonly _tree: Tree
  private readonly _parent: No | null
  private _open: boolean
  private _loading: boolean
  public readonly address: Array<number>
  public readonly name: string
  public readonly nodes: Array<No>
  public readonly badge?: BadgeProps
  public readonly value: any

  public constructor(
    data: TreeviewItem[number],
    parent: No | null,
    tree: Tree
  ) {
    this.address = data.address
    this.name = data.title
    this.nodes = data.items
      ? data.items.map(
          (item, i) =>
            new No({ ...item, address: [...data.address, i] }, this, tree)
        )
      : []
    this._open = data.expanded ?? false
    this.badge = data.badge
    this._parent = parent
    this._tree = tree
    this.value = data.value
    this._loading = data.loading ?? false
  }

  public isOpen(): boolean {
    return this._open
  }

  public isLoading(): boolean {
    return this._open
  }

  public parent(): No | null {
    return this._parent
  }

  public close(): void {
    this._open = false
  }

  public toggleOpen(): void {
    if (!this.isLeaf()) {
      if (!this._open) {
        this.nodes.forEach(n => n.close())
      }

      this._open = !this._open

      if (!this._open && this._tree.isChildNodeFocused(this.address)) {
        this._tree.focusNode(this)
      }
    }
  }

  public isLeaf(): boolean {
    return this.nodes.length === 0
  }

  public isExpanded(): boolean {
    return this._open && !this.isLeaf()
  }

  public addressToString(): string {
    return this.address.join('-')
  }

  public visibleChildren(): Array<No> {
    return this._open ? this.nodes : []
  }

  public addNodes(data: TreeviewItem): void {
    data.map(d => this.nodes.push(new No(d, this, this._tree)))
  }

  public toggleLoading(): void {
    this._loading = !this._loading
  }
}

Answer 1

It seems upon updates by the OP that I glommed onto the wrong part of the text and the CQS aspect of this is not terribly relevant to the real question. I'm going to leave the original answer since it was upvoted and might be useful to others.

As I understand it now, you are asking about whether it is important to wrap your properties in methods as is common in Java code. I don't think there's a simple yes or no answer. I would also offer that the really important questions are whether your state is encapsulated sufficiently and whether your components are too tightly coupled.

I make that distinction because while using methods to access properties can be useful in encapsulating state, wrapping a property in a 'get' method doesn't necessarily encapsulate state effectively.

As an example of this, consider a JS const array variable. No one can change the value of the variable even if it is public. But that in no way prevents the array from being modified. Any part of your code can modify it. You can make it private and provide an access method but that doesn't prevent external modification of the array either. In both cases you are still giving direct access to the underlying array which is part of the state. And if you set that array reference to one passed in from elsewhere, that source still can modify the array and change the state of the object.

I'm not very familiar with TS but it seems you have this kind of situation in your second code snippet e.g. public readonly address: Array<number>

To address this, there are a couple of options. You can freeze an array that is exposed as readonly. This might be a good option if you never want the contents of the array to change. If you want your object to be the only thing that can change the array, you can use an accessor method and use defensive copies (on initialization and in the accessor) to prevent other parts of the code from making changes to the array which makes up part of your objects internal state.

Original Answer

As noted in the comments, this idea is related to the principle of Command-Query Separation (CQS). We can break this into two parts:

1. Querying (getting) an object shouldn't change its state.
2. Updating a value should not return its state.

I would say that the first is a pretty rock-solid rule. You are just asking for problems if you violate this. Maybe there's an exception but I can't think of one on a Monday.

As far as the second one goes, there are a number of reasons you might want to ignore that, or at least not return void:

Method Chaining

It's common in contemporary code to return this/self instead of void/None. This allows you to write code like this:

object.foo(x).bar(y) instead of:

object.foo(x)
object.bar(y)

I would argue that this still follows the spirit of CQS because the return is not really querying the object. However, this approach also often involves factory methods where an object will create a new child object and return it. This would seem to violate CQS but I have no issue with it and I think it's much better than the alternatives.

Success Status

Some APIs I've worked will do things like return a boolean flag indicating whether the call resulted in a change. For example, adding to a set will return true if the set was changed and false if the item was already in the set. This is useful when you want to take a one-time action upon a new value. Similarly, some map/dictionary APIs will return the prior mapped value on an update. Doing this with two calls might create flaws (e.g. TOCTOU) in some contexts such as discussed in the next section.

Atomicity

I would argue that the most clearly appropriate reason to ignore CQS (for updates) is when you can't guarantee that some other change will not occur between a read and a write. For example, lets say you have a multi-threaded routine which adds items to a list. Each thread adds an item and then if the number of items and fire a notification on each hundredth item. If you need to do this in two calls, there's a potential issue: thread A adds an item making the total 500. Before thread A can get the total count, thread B adds an item making the total 501. Then both thread A and thread B get the count as 501. Therefore no event is fired on the 500th item.

To solve this under a strict CQS regime, you would need some sort of locking mechanism preventing other threads from making changes between calls. Aside from being complicated and error prone, it can create an issue with contention. If instead, you have that add method return the count of items, you can allow the method to handle thread safety internally with a much broader array of approaches.

Answer 2

Should you use methods over properties?

Properties are kind of methods already. But, practically, if you do this you will run into problems with serialisation libraries and the like (vue observables) which like to map to properties, not methods.

Also, its somewhat clumsy in the general sense. Your dog class for example, it needs walk(), run(), trot()? instead of enum Gait {get;set;} It works well for well defined cases and is inheritance friendly, but hard to change on a whim.

return void

Always makes testing hard.

Verbs and Nouns

No, I mean you have already confused things with walk both a noun and a verb and then resorted to multi word isLeaf isExpanded etc on tree. English is too vague for those kind of rules to convey meaning. Just choose the best term you can think of, don't limit yourself.

Would you guys agree with that and recommend it

For well defined OOP style classes, like your Tree which hang around in memory and have internal state machines. Yes.

• You are exposing the levers of the machine and the levers won't change.
• Inheritance works well for these type of objects and the methods will work well with inheritance

For general business objects? No.

• You are going to want to serialise these, put them on databases, send them around, map user interfaces to them etc
• They are susceptible to frequent change.
• Inheritance is out of favour and this is a very OOP style construct.