He estado leyendo Trabajar eficazmente con código heredado y código limpio con el objetivo de aprender estrategias sobre cómo comenzar a limpiar la base de código existente de una aplicación de formularios web ASP.NET de gran tamaño.
Este sistema existe desde 2005 y desde entonces ha experimentado una serie de mejoras. Originalmente, el código se estructuraba de la siguiente manera (y todavía se estructura en gran medida de esta manera):
- ASP.NET (aspx / ascx)
- Código subyacente (c #)
- Capa de lógica empresarial (c #)
- Capa de acceso a datos (c #)
- Base de datos (Oracle)
El problema principal es que el código está enmascarado por procedimientos como orientado a objetos. Prácticamente viola todas las pautas descritas en ambos libros.
Este es un ejemplo de una clase típica en la capa de lógica de negocios:
public class AddressBO
{
public TransferObject GetAddress(string addressID)
{
if (StringUtils.IsNull(addressID))
{
throw new ValidationException("Address ID must be entered");
}
AddressDAO addressDAO = new AddressDAO();
return addressDAO.GetAddress(addressID);
}
public TransferObject Insert(TransferObject addressDetails)
{
if (StringUtils.IsNull(addressDetails.GetString("EVENT_ID")) ||
StringUtils.IsNull(addressDetails.GetString("LOCALITY")) ||
StringUtils.IsNull(addressDetails.GetString("ADDRESS_TARGET")) ||
StringUtils.IsNull(addressDetails.GetString("ADDRESS_TYPE_CODE")) ||
StringUtils.IsNull(addressDetails.GetString("CREATED_BY")))
{
throw new ValidationException(
"You must enter an Event ID, Locality, Address Target, Address Type Code and Created By.");
}
string addressID = Sequence.GetNextValue("ADDRESS_ID_SEQ");
addressDetails.SetValue("ADDRESS_ID", addressID);
string syncID = Sequence.GetNextValue("SYNC_ID_SEQ");
addressDetails.SetValue("SYNC_ADDRESS_ID", syncID);
TransferObject syncDetails = new TransferObject();
Transaction transaction = new Transaction();
try
{
AddressDAO addressDAO = new AddressDAO();
addressDAO.Insert(addressDetails, transaction);
// insert the record for the target
TransferObject addressTargetDetails = new TransferObject();
switch (addressDetails.GetString("ADDRESS_TARGET"))
{
case "PARTY_ADDRESSES":
{
addressTargetDetails.SetValue("ADDRESS_ID", addressID);
addressTargetDetails.SetValue("ADDRESS_TYPE_CODE",
addressDetails.GetString("ADDRESS_TYPE_CODE"));
addressTargetDetails.SetValue("PARTY_ID", addressDetails.GetString("PARTY_ID"));
addressTargetDetails.SetValue("EVENT_ID", addressDetails.GetString("EVENT_ID"));
addressTargetDetails.SetValue("CREATED_BY", addressDetails.GetString("CREATED_BY"));
addressDAO.InsertPartyAddress(addressTargetDetails, transaction);
break;
}
case "PARTY_CONTACT_ADDRESSES":
{
addressTargetDetails.SetValue("ADDRESS_ID", addressID);
addressTargetDetails.SetValue("ADDRESS_TYPE_CODE",
addressDetails.GetString("ADDRESS_TYPE_CODE"));
addressTargetDetails.SetValue("PUBLIC_RELEASE_FLAG",
addressDetails.GetString("PUBLIC_RELEASE_FLAG"));
addressTargetDetails.SetValue("CONTACT_ID", addressDetails.GetString("CONTACT_ID"));
addressTargetDetails.SetValue("EVENT_ID", addressDetails.GetString("EVENT_ID"));
addressTargetDetails.SetValue("CREATED_BY", addressDetails.GetString("CREATED_BY"));
addressDAO.InsertContactAddress(addressTargetDetails, transaction);
break;
}
<< many more cases here >>
default:
{
break;
}
}
// synchronise
SynchronisationBO synchronisationBO = new SynchronisationBO();
syncDetails = synchronisationBO.Synchronise("I", transaction,
"ADDRESSES", addressDetails.GetString("ADDRESS_TARGET"),
addressDetails, addressTargetDetails);
// commit
transaction.Commit();
}
catch (Exception)
{
transaction.Rollback();
throw;
}
return new TransferObject("ADDRESS_ID", addressID, "SYNC_DETAILS", syncDetails);
}
<< many more methods are here >>
}Tiene mucha duplicación, la clase tiene una serie de responsabilidades, etc., etc., generalmente es un código "no limpio".
Todo el código en todo el sistema depende de implementaciones concretas.
Este es un ejemplo de una clase típica en la capa de acceso a datos:
public class AddressDAO : GenericDAO
{
public static readonly string BASE_SQL_ADDRESSES =
"SELECT " +
" a.address_id, " +
" a.event_id, " +
" a.flat_unit_type_code, " +
" fut.description as flat_unit_description, " +
" a.flat_unit_num, " +
" a.floor_level_code, " +
" fl.description as floor_level_description, " +
" a.floor_level_num, " +
" a.building_name, " +
" a.lot_number, " +
" a.street_number, " +
" a.street_name, " +
" a.street_type_code, " +
" st.description as street_type_description, " +
" a.street_suffix_code, " +
" ss.description as street_suffix_description, " +
" a.postal_delivery_type_code, " +
" pdt.description as postal_delivery_description, " +
" a.postal_delivery_num, " +
" a.locality, " +
" a.state_code, " +
" s.description as state_description, " +
" a.postcode, " +
" a.country, " +
" a.lock_num, " +
" a.created_by, " +
" TO_CHAR(a.created_datetime, '" + SQL_DATETIME_FORMAT + "') as created_datetime, " +
" a.last_updated_by, " +
" TO_CHAR(a.last_updated_datetime, '" + SQL_DATETIME_FORMAT + "') as last_updated_datetime, " +
" a.sync_address_id, " +
" a.lat," +
" a.lon, " +
" a.validation_confidence, " +
" a.validation_quality, " +
" a.validation_status " +
"FROM ADDRESSES a, FLAT_UNIT_TYPES fut, FLOOR_LEVELS fl, STREET_TYPES st, " +
" STREET_SUFFIXES ss, POSTAL_DELIVERY_TYPES pdt, STATES s " +
"WHERE a.flat_unit_type_code = fut.flat_unit_type_code(+) " +
"AND a.floor_level_code = fl.floor_level_code(+) " +
"AND a.street_type_code = st.street_type_code(+) " +
"AND a.street_suffix_code = ss.street_suffix_code(+) " +
"AND a.postal_delivery_type_code = pdt.postal_delivery_type_code(+) " +
"AND a.state_code = s.state_code(+) ";
public TransferObject GetAddress(string addressID)
{
//Build the SELECT Statement
StringBuilder selectStatement = new StringBuilder(BASE_SQL_ADDRESSES);
//Add WHERE condition
selectStatement.Append(" AND a.address_id = :addressID");
ArrayList parameters = new ArrayList{DBUtils.CreateOracleParameter("addressID", OracleDbType.Decimal, addressID)};
// Execute the SELECT statement
Query query = new Query();
DataSet results = query.Execute(selectStatement.ToString(), parameters);
// Check if 0 or more than one rows returned
if (results.Tables[0].Rows.Count == 0)
{
throw new NoDataFoundException();
}
if (results.Tables[0].Rows.Count > 1)
{
throw new TooManyRowsException();
}
// Return a TransferObject containing the values
return new TransferObject(results);
}
public void Insert(TransferObject insertValues, Transaction transaction)
{
// Store Values
string addressID = insertValues.GetString("ADDRESS_ID");
string syncAddressID = insertValues.GetString("SYNC_ADDRESS_ID");
string eventID = insertValues.GetString("EVENT_ID");
string createdBy = insertValues.GetString("CREATED_BY");
// postal delivery
string postalDeliveryTypeCode = insertValues.GetString("POSTAL_DELIVERY_TYPE_CODE");
string postalDeliveryNum = insertValues.GetString("POSTAL_DELIVERY_NUM");
// unit/building
string flatUnitTypeCode = insertValues.GetString("FLAT_UNIT_TYPE_CODE");
string flatUnitNum = insertValues.GetString("FLAT_UNIT_NUM");
string floorLevelCode = insertValues.GetString("FLOOR_LEVEL_CODE");
string floorLevelNum = insertValues.GetString("FLOOR_LEVEL_NUM");
string buildingName = insertValues.GetString("BUILDING_NAME");
// street
string lotNumber = insertValues.GetString("LOT_NUMBER");
string streetNumber = insertValues.GetString("STREET_NUMBER");
string streetName = insertValues.GetString("STREET_NAME");
string streetTypeCode = insertValues.GetString("STREET_TYPE_CODE");
string streetSuffixCode = insertValues.GetString("STREET_SUFFIX_CODE");
// locality/state/postcode/country
string locality = insertValues.GetString("LOCALITY");
string stateCode = insertValues.GetString("STATE_CODE");
string postcode = insertValues.GetString("POSTCODE");
string country = insertValues.GetString("COUNTRY");
// esms address
string esmsAddress = insertValues.GetString("ESMS_ADDRESS");
//address/GPS
string lat = insertValues.GetString("LAT");
string lon = insertValues.GetString("LON");
string zoom = insertValues.GetString("ZOOM");
//string validateDate = insertValues.GetString("VALIDATED_DATE");
string validatedBy = insertValues.GetString("VALIDATED_BY");
string confidence = insertValues.GetString("VALIDATION_CONFIDENCE");
string status = insertValues.GetString("VALIDATION_STATUS");
string quality = insertValues.GetString("VALIDATION_QUALITY");
// the insert statement
StringBuilder insertStatement = new StringBuilder("INSERT INTO ADDRESSES (");
StringBuilder valuesStatement = new StringBuilder("VALUES (");
ArrayList parameters = new ArrayList();
// build the insert statement
insertStatement.Append("ADDRESS_ID, EVENT_ID, CREATED_BY, CREATED_DATETIME, LOCK_NUM ");
valuesStatement.Append(":addressID, :eventID, :createdBy, SYSDATE, 1 ");
parameters.Add(DBUtils.CreateOracleParameter("addressID", OracleDbType.Decimal, addressID));
parameters.Add(DBUtils.CreateOracleParameter("eventID", OracleDbType.Decimal, eventID));
parameters.Add(DBUtils.CreateOracleParameter("createdBy", OracleDbType.Varchar2, createdBy));
// build the insert statement
if (!StringUtils.IsNull(syncAddressID))
{
insertStatement.Append(", SYNC_ADDRESS_ID");
valuesStatement.Append(", :syncAddressID");
parameters.Add(DBUtils.CreateOracleParameter("syncAddressID", OracleDbType.Decimal, syncAddressID));
}
if (!StringUtils.IsNull(postalDeliveryTypeCode))
{
insertStatement.Append(", POSTAL_DELIVERY_TYPE_CODE");
valuesStatement.Append(", :postalDeliveryTypeCode ");
parameters.Add(DBUtils.CreateOracleParameter("postalDeliveryTypeCode", OracleDbType.Varchar2, postalDeliveryTypeCode));
}
if (!StringUtils.IsNull(postalDeliveryNum))
{
insertStatement.Append(", POSTAL_DELIVERY_NUM");
valuesStatement.Append(", :postalDeliveryNum ");
parameters.Add(DBUtils.CreateOracleParameter("postalDeliveryNum", OracleDbType.Varchar2, postalDeliveryNum));
}
if (!StringUtils.IsNull(flatUnitTypeCode))
{
insertStatement.Append(", FLAT_UNIT_TYPE_CODE");
valuesStatement.Append(", :flatUnitTypeCode ");
parameters.Add(DBUtils.CreateOracleParameter("flatUnitTypeCode", OracleDbType.Varchar2, flatUnitTypeCode));
}
if (!StringUtils.IsNull(lat))
{
insertStatement.Append(", LAT");
valuesStatement.Append(", :lat ");
parameters.Add(DBUtils.CreateOracleParameter("lat", OracleDbType.Decimal, lat));
}
if (!StringUtils.IsNull(lon))
{
insertStatement.Append(", LON");
valuesStatement.Append(", :lon ");
parameters.Add(DBUtils.CreateOracleParameter("lon", OracleDbType.Decimal, lon));
}
if (!StringUtils.IsNull(zoom))
{
insertStatement.Append(", ZOOM");
valuesStatement.Append(", :zoom ");
parameters.Add(DBUtils.CreateOracleParameter("zoom", OracleDbType.Decimal, zoom));
}
if (!StringUtils.IsNull(flatUnitNum))
{
insertStatement.Append(", FLAT_UNIT_NUM");
valuesStatement.Append(", :flatUnitNum ");
parameters.Add(DBUtils.CreateOracleParameter("flatUnitNum", OracleDbType.Varchar2, flatUnitNum));
}
if (!StringUtils.IsNull(floorLevelCode))
{
insertStatement.Append(", FLOOR_LEVEL_CODE");
valuesStatement.Append(", :floorLevelCode ");
parameters.Add(DBUtils.CreateOracleParameter("floorLevelCode", OracleDbType.Varchar2, floorLevelCode));
}
if (!StringUtils.IsNull(floorLevelNum))
{
insertStatement.Append(", FLOOR_LEVEL_NUM");
valuesStatement.Append(", :floorLevelNum ");
parameters.Add(DBUtils.CreateOracleParameter("floorLevelNum", OracleDbType.Varchar2, floorLevelNum));
}
if (!StringUtils.IsNull(buildingName))
{
insertStatement.Append(", BUILDING_NAME");
valuesStatement.Append(", :buildingName ");
parameters.Add(DBUtils.CreateOracleParameter("buildingName", OracleDbType.Varchar2, buildingName));
}
if (!StringUtils.IsNull(lotNumber))
{
insertStatement.Append(", LOT_NUMBER");
valuesStatement.Append(", :lotNumber ");
parameters.Add(DBUtils.CreateOracleParameter("lotNumber", OracleDbType.Varchar2, lotNumber));
}
if (!StringUtils.IsNull(streetNumber))
{
insertStatement.Append(", STREET_NUMBER");
valuesStatement.Append(", :streetNumber ");
parameters.Add(DBUtils.CreateOracleParameter("streetNumber", OracleDbType.Varchar2, streetNumber));
}
if (!StringUtils.IsNull(streetName))
{
insertStatement.Append(", STREET_NAME");
valuesStatement.Append(", :streetName ");
parameters.Add(DBUtils.CreateOracleParameter("streetName", OracleDbType.Varchar2, streetName));
}
if (!StringUtils.IsNull(streetTypeCode))
{
insertStatement.Append(", STREET_TYPE_CODE");
valuesStatement.Append(", :streetTypeCode ");
parameters.Add(DBUtils.CreateOracleParameter("streetTypeCode", OracleDbType.Varchar2, streetTypeCode));
}
if (!StringUtils.IsNull(streetSuffixCode))
{
insertStatement.Append(", STREET_SUFFIX_CODE");
valuesStatement.Append(", :streetSuffixCode ");
parameters.Add(DBUtils.CreateOracleParameter("streetSuffixCode", OracleDbType.Varchar2, streetSuffixCode));
}
if (!StringUtils.IsNull(locality))
{
insertStatement.Append(", LOCALITY");
valuesStatement.Append(", :locality");
parameters.Add(DBUtils.CreateOracleParameter("locality", OracleDbType.Varchar2, locality));
}
if (!StringUtils.IsNull(stateCode))
{
insertStatement.Append(", STATE_CODE");
valuesStatement.Append(", :stateCode");
parameters.Add(DBUtils.CreateOracleParameter("stateCode", OracleDbType.Varchar2, stateCode));
}
if (!StringUtils.IsNull(postcode))
{
insertStatement.Append(", POSTCODE");
valuesStatement.Append(", :postcode ");
parameters.Add(DBUtils.CreateOracleParameter("postcode", OracleDbType.Varchar2, postcode));
}
if (!StringUtils.IsNull(country))
{
insertStatement.Append(", COUNTRY");
valuesStatement.Append(", :country ");
parameters.Add(DBUtils.CreateOracleParameter("country", OracleDbType.Varchar2, country));
}
if (!StringUtils.IsNull(esmsAddress))
{
insertStatement.Append(", ESMS_ADDRESS");
valuesStatement.Append(", :esmsAddress ");
parameters.Add(DBUtils.CreateOracleParameter("esmsAddress", OracleDbType.Varchar2, esmsAddress));
}
if (!StringUtils.IsNull(validatedBy))
{
insertStatement.Append(", VALIDATED_DATE");
valuesStatement.Append(", SYSDATE ");
insertStatement.Append(", VALIDATED_BY");
valuesStatement.Append(", :validatedBy ");
parameters.Add(DBUtils.CreateOracleParameter("validatedBy", OracleDbType.Varchar2, validatedBy));
}
if (!StringUtils.IsNull(confidence))
{
insertStatement.Append(", VALIDATION_CONFIDENCE");
valuesStatement.Append(", :confidence ");
parameters.Add(DBUtils.CreateOracleParameter("confidence", OracleDbType.Decimal, confidence));
}
if (!StringUtils.IsNull(status))
{
insertStatement.Append(", VALIDATION_STATUS");
valuesStatement.Append(", :status ");
parameters.Add(DBUtils.CreateOracleParameter("status", OracleDbType.Varchar2, status));
}
if (!StringUtils.IsNull(quality))
{
insertStatement.Append(", VALIDATION_QUALITY");
valuesStatement.Append(", :quality ");
parameters.Add(DBUtils.CreateOracleParameter("quality", OracleDbType.Decimal, quality));
}
// finish off the statement
insertStatement.Append(") ");
valuesStatement.Append(")");
// build the insert statement
string sql = insertStatement + valuesStatement.ToString();
// Execute the INSERT Statement
Dml dmlDAO = new Dml();
int rowsAffected = dmlDAO.Execute(sql, transaction, parameters);
if (rowsAffected == 0)
{
throw new NoRowsAffectedException();
}
}
<< many more methods go here >>
}Este sistema fue desarrollado por mí y un pequeño equipo en 2005 después de un curso de .NET de 1 semana. Antes que mi experiencia fue en aplicaciones cliente-servidor. En los últimos 5 años, he llegado a reconocer los beneficios de las pruebas unitarias automatizadas, las pruebas de integración automatizadas y las pruebas de aceptación automatizadas (utilizando Selenium o equivalente), pero la base de código actual parece imposible de introducir estos conceptos.
Ahora estamos comenzando a trabajar en un importante proyecto de mejora con plazos ajustados. El equipo consta de 5 desarrolladores .NET: 2 desarrolladores con algunos años de experiencia .NET y otros 3 con poca o ninguna experiencia .NET. Ninguno de los miembros del equipo (incluido yo mismo) tiene experiencia en el uso de pruebas de unidades .NET o marcos de imitación.
¿Qué estrategia usarías para hacer este código más limpio, más orientado a objetos, comprobable y mantenible?
c#
.net
unit-testing
object-oriented
Antonio
fuente
fuente
Respuestas:
Usted menciona dos libros en los que uno de los mensajes principales es "La regla de los Boy Scouts", es decir, limpie el código mientras lo toca. Si tiene un sistema de trabajo, una reescritura masiva es contraproducente. En cambio, a medida que agrega nuevas funciones, asegúrese de mejorar el código tal como está.
Para profundizar más, Feathers habla sobre probar la aplicación por completo: los puntos lógicos en los que se conectan las unidades. Puede aprovechar una costura para crear un trozo o una simulación para una dependencia, de modo que pueda escribir pruebas alrededor del objeto dependiente. Tomemos su AddressBO como ejemplo
Hay una costura obvia entre AddressBO y AddressDAO. Creemos una interfaz para AddressDAO y permitamos que la dependencia se inyecte en AddressBO.
Ahora usted prepara su AddressBO para permitir la inyección
Aquí estamos usando la "inyección de dependencia del pobre". Nuestro único objetivo es romper la costura y permitirnos probar AddressBO. Ahora en nuestras pruebas unitarias podemos hacer un simulacro IAddressDAO y validar las interacciones entre los dos objetos.
fuente
Si no recuerdo mal, trabajar eficazmente con código heredado dice que una reescritura completa no garantiza que el nuevo código sea mejor que el anterior (desde un punto de vista de funcionalidad / defectos). Las refactorizaciones en ese libro son para corregir errores / agregar nuevas características.
Otro libro que recomendaría es Brownfield Application Development en .NET, que básicamente dice que no se debe reescribir también. Habla sobre hacer cambios constantes e iterativos cada vez que agrega nuevas funciones o corrige defectos. Aborda las consideraciones de costo versus beneficios y advierte sobre tratar de morder demasiado a la vez. Si bien Trabajar eficazmente con Legacy Code habla principalmente sobre cómo refactorizar en el nivel de micro / código, Brownfield Application Development en .NET , en su mayoría cubre las consideraciones de nivel superior al re-factorizar (junto con algunas cosas de nivel de código también).
El libro de Brownfield también sugiere averiguar qué área del código le está causando más problemas y centrarse allí. Puede que no valga la pena cambiar cualquier otra área que no necesite mucho mantenimiento.
fuente
Para una aplicación de este tipo, es mucho más rentable comenzar cubriéndola con pruebas de integración (automatizadas) de nivel superior en lugar de pruebas unitarias. Luego, con las pruebas de integración como su red de seguridad, puede comenzar a refactorizar en pequeños pasos si es apropiado, es decir, si el costo de la refactorización se amortiza a largo plazo. Como otros han señalado, esto no es evidente por sí mismo.
Vea también esta respuesta anterior mía a una pregunta similar; Esperamos que te sea útil.
fuente
Tenga mucho cuidado al tirar y reescribir el código de ejecución ( Cosas que nunca debe hacer ). Claro que puede ser feo, pero si funciona, déjalo. Vea la publicación del blog de Joel, seguro que tiene más de 10 años, pero todavía está en el objetivo.
fuente
Como dijo Mike, la 'regla de boy scout' es probablemente la mejor aquí, si el código funciona y no es una fuente constante de informes de errores, preferiría dejarlo allí y mejorarlo lentamente con el tiempo.
Durante su proyecto de mejora, permita nuevas formas de hacer las cosas. Por ejemplo, use un ORM para nuevas características y omita el patrón de capa de datos existente. Cuando encuentre mejoras que necesiten tocar el código existente, puede mover parte del código relacionado a la nueva forma de hacer las cosas. Usar una fachada o algunos adaptadores en algunos lugares puede ayudarlo a aislar el código anterior, tal vez incluso por capa. Esto podría ayudarlo a dejar pasar el código antiguo a través del tiempo.
Del mismo modo, esto puede ayudarlo a agregar pruebas unitarias, puede comenzar con el nuevo código que crea y agregar lentamente algunas pruebas para el código antiguo que debe tocar para las nuevas mejoras.
fuente
Ambos son buenos libros. Si va a comenzar a reescribir el código de esa manera, creo que es importante comenzar también a cubrir el código con pruebas unitarias para ayudarlo a mantenerse estable a medida que lo reescribe.
Tiene que hacerse en pequeños pasos y modificar ese tipo de código puede desestabilizar fácilmente todo el sistema.
No modificaría ningún código en el que no esté trabajando activamente. Solo haga esto en el código que está mejorando o arreglando activamente. Si algo está cumpliendo su propósito pero no se ha modificado en años, simplemente déjelo. Está haciendo su cosa, incluso si conoces una mejor manera.
Al final del día, la empresa necesita productividad. Si bien un mejor código aumenta la productividad, el código de reescritura solo porque podría escribirse mejor probablemente no sea la mejor manera de aportar valor a su producto.
fuente