4

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<No>) => { nos.forEach(no => { 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 < 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 < 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<No>) => { for (const node of nodes) { if (node.isOpen() && !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 } }
Improve this question
10
  • 3
    en.wikipedia.org/wiki/Command%E2%80%93query_separation Commented Jun 9 at 16:38
  • 3
    Ideally, shouldn't expose internal state to help maintain encapsulation. If some code outside the object is doing inquiry about its internal state to decide on some action, then consider that logic should maybe be moved into the object itself. martinfowler.com/bliki/TellDontAsk.html Commented Jun 9 at 16:51
  • @ErikEidt, that makes a lot of sense. It's just that I am trying to apply that to the frontend, so I need to show some properties as content. I still haven't figured a way to do that better then by adding the TS keyword readonly to them, so that at dev-time, collaborators won't change those values. Commented Jun 9 at 17:00
  • 1
    "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." - that may be so by some convention that you and your team follow, but language-wise, methods and properties aren't fundamentally different (a get/set property is really just syntactic sugar for a pair of methods under the hood). It all depends on what you return from either (direct reference, a copy, a transformed value, wrapped value, a truly immutable value, etc.) Commented Jun 10 at 19:08
  • 1
    P.S. One thing that may be confusing for design novices is what exactly is "internal state". Well that's something you have to decide on when you create the class (or component or whatever). You have to think it through and decide on two things: what methods and data types are going to appear as part of the public interface (public methods and properties, their input and return types) that client (calling) code is free to make use of and rely upon, vs what part of the data you're going to treat as internal representation (that you may want to tweak later on). Then you encapsulate that. Commented Jun 10 at 19:13

2 Answers 2

Reset to default
12

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.

Improve this answer
7
  • 1
    I've updated my question to add a bit of what I am doing. Commented Jun 9 at 19:25
  • 1
    Regarding querying possibly mutating state, internally tracked performance counters come to mind as an example of when this may make sense. Stuff like tracking the number of times that a given part of the state is queried. It may make more sense to use a proxy object for the tracking, but OTOH that has it’s own downsides compared to just doing the tracking in the actual object itself. Commented Jun 10 at 11:04
  • 1
    @AustinHemmelgarn Yes. I considered mentioning this in the answer but I decided it might be too confusing. I think there's a bright line distinction between the objects logical state and 'accounting' state but I find it hard to explain concisely. It's similar in reasoning as to why even though Safe methods in HTTP "have a read-only semantic, servers can alter their state: e.g., they can log or keep statistics." Commented Jun 10 at 15:13
  • @JimmyJamessupportsCanada, I guess I just stumbled upon an example. While creating methods in a class for making requests, I discovered I cannot have a method name delete (a verb) which will return the status of the request (I am using the Either pattern). In this case, each method should return an object Request perhaps which would have its state; or have a class for each operation I need (postX, getY, deleteZ), each one being a request. Commented Jun 11 at 14:50
  • @BernardoBeniniFantin Sorry, can you clarify what that is an example of, exactly? Commented Jun 11 at 16:03
4

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.
Improve this answer
2
  • 1
    About hte walk noun/verb, I am writing code in another language, in which verbs and nouns have a clearer distinction, so I wouldn't worry about that. About void making tests harder, what should I do instead? I mean, I have many, non-CQRS methods that return void simply because they return nothing. Commented Jun 9 at 22:12
  • "Properties are kind of methods already." - notice the question was asked in terms of TypeScript, where "property" means both data (storage) and accessor (getter/setter) properties (and technically even encompasses methods, as "function-valued properties"). Commented Jun 10 at 6:51

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.