Problema de estilo de codificación: ¿Deberíamos tener funciones que tomen un parámetro, lo modifiquen y luego DEVUELTAN ese parámetro?

Tengo un pequeño debate con mi amigo sobre si estas dos prácticas son simplemente dos caras de la misma moneda, o si una es realmente mejor.

Tenemos una función que toma un parámetro, completa un miembro y luego lo devuelve:

Item predictPrice(Item item)

Creo que, dado que funciona en el mismo objeto que se pasa, no tiene sentido devolver el artículo. De hecho, en todo caso, desde la perspectiva de la persona que llama, confunde las cosas, ya que podría esperar que devuelva un nuevo elemento que no lo hace.

Afirma que no hay diferencia, y que ni siquiera importaría si creara un nuevo Artículo y lo devolviera. Estoy totalmente en desacuerdo por las siguientes razones:

• Si tiene múltiples referencias al elemento que pasa (o punteros o lo que sea), asignar un nuevo objeto y devolverlo es de importancia material ya que esas referencias serán incorrectas.

• En lenguajes no gestionados por memoria, la función que asigna una nueva instancia reclama la propiedad de la memoria y, por lo tanto, tendríamos que implementar un método de limpieza que se llama en algún momento.

• Asignar en el montón es potencialmente costoso y, por lo tanto, es importante si la función llamada lo hace.

Por lo tanto, creo que es muy importante poder ver a través de una firma de métodos si modifica un objeto o asigna uno nuevo. Como resultado, creo que como la función simplemente modifica el objeto pasado, la firma debería ser:

void predictPrice(Item item)

En cada base de código (ciertamente, bases de código C y C ++, no en Java, que es el lenguaje en el que estamos trabajando) con las que he trabajado, el estilo anterior se ha cumplido esencialmente y se ha mantenido por programadores considerablemente más experimentados. Afirma que, dado que el tamaño de mi muestra de bases de código y colegas es pequeño de todas las posibles bases de código y colegas, y por lo tanto, mi experiencia no es un verdadero indicador sobre si uno es superior.

Entonces, ¿alguna idea?

Esto realmente es una cuestión de opinión, pero para lo que vale, me resulta engañoso devolver el artículo modificado si el artículo se modifica en su lugar. Por separado, si predictPriceva a modificar el elemento, debe tener un nombre que indique que va a hacer eso (como setPredictPriceo algo así).

Preferiría (en orden)

1. Ese predictPricefue un método deItem (módulo una buena razón para que no lo sea, que bien podría haber en su diseño general) que

   • Devuelto el precio previsto, o

   • Tenía un nombre como en su setPredictedPricelugar

2. Eso predictPrice no modificó Item, pero devolvió el precio previsto

3. Ese predictPriceera un voidmétodo llamadosetPredictedPrice

4. Eso predictPriceregresó this(para el método de encadenamiento a otros métodos en cualquier instancia de la que sea parte) (y fue llamado setPredictedPrice)

Dentro de un paradigma de diseño orientado a objetos, las cosas no deberían estar modificando objetos fuera del objeto mismo. Cualquier cambio en el estado de un objeto debe hacerse a través de métodos en el objeto.

Por lo tanto, void predictPrice(Item item)como función miembro de alguna otra clase está mal . Podría haber sido aceptable en los días de C, pero para Java y C ++, las modificaciones de un objeto implican un acoplamiento más profundo con el objeto que probablemente conduciría a otros problemas de diseño en el futuro (cuando refactoriza la clase y cambia sus campos, ahora que 'predicPrice' debe cambiarse en algún otro archivo.

Devolver un nuevo objeto, no tiene efectos secundarios asociados, el parámetro pasado no cambia. Usted (el método predictPrice) no sabe dónde más se usa ese parámetro. ¿Es Itemuna clave para un hash en alguna parte? ¿Cambiaste su código hash al hacer esto? ¿Alguien más lo está esperando esperando que no cambie?

Estos problemas de diseño son los que sugieren fuertemente que no debería estar modificando objetos (yo argumentaría por la inmutabilidad en muchos casos), y si lo hace, los cambios en el estado deberían estar contenidos y controlados por el propio objeto en lugar de algo más fuera de la clase.

Veamos qué sucede si uno juega con los campos de algo en un hash. Tomemos un código:

import java.util.*;

public class Main {
    public static void main (String[] args) {
        Set set = new HashSet();
        Data d = new Data(1,"foo");
        set.add(d);
        set.add(new Data(2,"bar"));
        System.out.println(set.contains(d));
        d.field1 = 2;
        System.out.println(set.contains(d));
    }

    public static class Data {
        int field1;
        String field2;

        Data(int f1, String f2) {
            field1 = f1;
            field2 = f2;
        }

        public int hashCode() {
            return field2.hashCode() + field1;
        }

        public boolean equals(Object o) {
            if(!(o instanceof Data)) return false;
            Data od = (Data)o;
            return od.field1 == this.field1 && od.field2.equals(this.field2);

        }
    }
}

^ideona

Y admito que este no es el mejor código (acceso directo a los campos), pero sirve para demostrar un problema con los datos mutables que se utilizan como clave para un HashMap, o en este caso, simplemente se ponen en un HashSet.

La salida de este código es:

true
false

Lo que sucedió es que el hashCode utilizado cuando se insertó es donde está el objeto en el hash. Cambiar los valores utilizados para calcular el hashCode no vuelve a calcular el hash en sí. Esto es un peligro de poner cualquier objeto mutable como clave para un hash.

Por lo tanto, volviendo al aspecto original de la pregunta, el método que se llama no "sabe" cómo se está utilizando el objeto que está obteniendo como parámetro. Proporcionar "permite mutar el objeto" como la única forma de hacer esto significa que hay una serie de errores sutiles que pueden colarse. _{^{Como perder valores en un hash ... a menos que agregue más y el hash se vuelva a agregar, confíe en mí , eso es un error desagradable para cazar (perdí el valor hasta que agregué 20 elementos más al hashMap y luego, de repente, vuelve a aparecer).}}

• A menos que haya una buena razón para modificar el objeto, devolver un objeto nuevo es lo más seguro.
• Cuando no es una buena razón para modificar el objeto, que la modificación debe ser realizada por el propio objeto (métodos que invocan) en lugar de a través de alguna función externa que puede girar sus campos.
  • Esto permite refactorizar el objeto con un menor costo de mantenimiento.
  • Esto permite que el objeto se asegure de que los valores que calculan su código hash no cambian (o no son parte de su cálculo de código hash)

Relacionado: sobrescribir y devolver el valor del argumento utilizado como condicional de una instrucción if, dentro de la misma instrucción if

Siempre sigo la práctica de no mutar el objeto de otra persona. Es decir, solo objetos mutantes propiedad de la clase / estructura / lo que está dentro.

Dada la siguiente clase de datos:

class Item {
    public double price = 0;
}

Esto esta bien:

class Foo {
    private Item bar = new Item();

    public void predictPrice() {
        bar.price = bar.price + 5; // or something
    }
}

// Elsewhere...

Foo foo = Foo();
foo.predictPrice();
System.out.println(foo.bar.price);
// Since I called predictPrice on Foo, which takes and returns nothing, it's
// apparent something might've changed

Y esto es muy claro:

class Foo {
    private Item bar = new Item();
}
class Baz {
    public double predictPrice(Item item) {
        return item.price + 5; // or something
    }
}

// Elsewhere...

Foo foo = Foo();
Baz baz = Baz();
foo.bar.price = baz.predictPrice(foo.bar);
System.out.println(foo.bar.price);
// Since I explicitly set foo.bar.price to the return value of predictPrice, it's
// obvious foo.bar.price might've changed

Pero esto es demasiado fangoso y misterioso para mis gustos:

class Foo {
    private Item bar = new Item();
}
class Baz {
    public void predictPrice(Item item) {
        item.price = item.price + 5; // or something
    }
}

// Elsewhere...

Foo foo = Foo();
Baz baz = Baz();
baz.predictPrice(foo.bar);
System.out.println(foo.bar.price);
// In my head, it doesn't appear that to foo, bar, or price changed. It seems to
// me that, if anything, I've placed a predicted price into baz that's based on
// the value of foo.bar.price

La sintaxis es diferente entre los diferentes idiomas, y no tiene sentido mostrar un código que no es específico de un idioma porque significa cosas completamente diferentes en diferentes idiomas.

En Java, Itemes un tipo de referencia: es el tipo de punteros a objetos que son instancias de Item. En C ++, este tipo se escribe como Item *(la sintaxis de la misma cosa es diferente entre los lenguajes). Entonces Item predictPrice(Item item)en Java es equivalente a Item *predictPrice(Item *item)en C ++. En ambos casos, es una función (o método) que toma un puntero a un objeto y devuelve un puntero a un objeto.

En C ++, Item(sin otra cosa) es un tipo de objeto (suponiendo que Itemes el nombre de una clase). Un valor de este tipo "es" un objeto y Item predictPrice(Item item)declara una función que toma un objeto y devuelve un objeto. Como &no se usa, se pasa y se devuelve por valor. Eso significa que el objeto se copia cuando se pasa y se copia cuando se devuelve. No hay equivalente en Java porque Java no tiene tipos de objeto.

Entonces cual es? Muchas de las cosas que hace en la pregunta (por ejemplo, "Creo que, dado que funciona en el mismo objeto que se pasa ...") depende de comprender exactamente lo que se pasa y se devuelve aquí.

La convención a la que he visto adherirse a las bases de código es preferir un estilo que sea claro a partir de la sintaxis. Si la función cambia de valor, prefiera pasar por puntero

void predictPrice(Item * const item);

Este estilo da como resultado:

• Al llamarlo ves:

  predicPrice (& my_item);

y sabes que será modificado por tu adherencia al estilo.

• De lo contrario, prefiera pasar por const ref o value cuando no desee que la función modifique la instancia.

Si bien no tengo una gran preferencia, votaría que devolver el objeto conduce a una mejor flexibilidad para una API.

Tenga en cuenta que solo tiene sentido mientras el método tiene libertad para devolver otro objeto. Si tu javadoc dice @returns the same value of parameter aque es inútil. Pero si su javadoc lo dice @return an instance that holds the same data than parameter a, devolver la misma instancia u otra es un detalle de implementación.

Por ejemplo, en mi proyecto actual (Java EE) tengo una capa empresarial; para almacenar en DB (y asignar una identificación automática) un empleado, por ejemplo, un empleado que tengo algo como

 public Employee createEmployee(Employee employeeData) {
   this.entityManager.persist(employeeData);
   return this.employeeData;
 }

Por supuesto, no hay diferencia con esta implementación porque JPA devuelve el mismo objeto después de asignar el Id. Ahora, si me cambié a JDBC

 public Employee createEmployee(Employee employeeData) {
   PreparedStatement ps = this.connection.prepareStatement("INSERT INTO EMPLOYEE(name, surname, data) VALUES(?, ?, ?)");
   ... // set parameters
   ps.execute();
   int id = getIdFromGeneratedKeys(ps);
   ??????
 }

Ahora en ????? Tengo tres opciones

1. Defina un setter para Id, y devolveré el mismo objeto. Feo, porquesetId estará disponible en todas partes.

2. Haga un trabajo pesado de reflexión y configure la identificación en el Employee objeto. Sucio, pero al menos se limita al createEmployeeregistro.

3. Proporcione un constructor que copie el original. Employee y acepte también el idconjunto.

4. Recupere el objeto de la base de datos (tal vez sea necesario si algunos valores de campo se calculan en la base de datos, o si desea llenar una relación real).

Ahora, para 1. y 2. puede devolver la misma instancia, pero para 3. y 4. devolverá nuevas instancias. Si desde el principio estableciste en tu API que el valor "real" será el que devuelve el método, tienes más libertad.

El único argumento real en el que puedo pensar es que, usando implementaciones 1. o 2., las personas que usan su API pueden pasar por alto la asignación del resultado y su código se rompería si cambia a implementaciones 3. o 4.).

De todos modos, como puede ver, mi preferencia se basa en otras preferencias personales (como prohibir un setter para Ids), por lo que tal vez no se aplique a todos.

Al codificar en Java, se debe intentar hacer que el uso de cada objeto de tipo mutable coincida con uno de dos patrones:

1. Exactamente una entidad se considera el "propietario" del objeto mutable y considera el estado de ese objeto como parte del propio. Otras entidades pueden tener referencias, pero deben considerar la referencia como la identificación de un objeto que es propiedad de otra persona.

2. A pesar de que el objeto es mutable, nadie que tenga una referencia a él puede mutarlo, lo que hace que la instancia sea efectivamente inmutable (tenga en cuenta que debido a las peculiaridades en el modelo de memoria de Java, no hay una buena manera de hacer que un objeto efectivamente inmutable tenga hilo- semántica inmutable segura sin agregar un nivel adicional de indirección a cada acceso). Cualquier entidad que encapsule el estado utilizando una referencia a dicho objeto y quiera cambiar el estado encapsulado debe crear un nuevo objeto que contenga el estado adecuado.

No me gusta que los métodos muten el objeto subyacente y devuelvan una referencia, ya que dan la apariencia de ajustarse al segundo patrón anterior. Hay algunos casos en los que el enfoque puede ser útil, pero el código que va a mutar objetos debería parecer que lo hará; el código similar se myThing = myThing.xyz(q);parece mucho menos a que muta el objeto identificado de lo myThingque lo haría simplemente myThing.xyz(q);.