J'ai lu Travailler efficacement avec le code hérité et le code propre dans le but d'apprendre des stratégies sur la façon de commencer à nettoyer la base de code existante d'une grande application de formulaires Web ASP.NET.
Ce système existe depuis 2005 et a depuis subi un certain nombre d'améliorations. À l'origine, le code était structuré comme suit (et est toujours largement structuré de cette façon):
- ASP.NET (aspx / ascx)
- Code-behind (c #)
- Couche logique métier (c #)
- Couche d'accès aux données (c #)
- Base de données (Oracle)
Le principal problème est que le code se fait passer pour un objet orienté objet. Il viole pratiquement toutes les directives décrites dans les deux livres.
Voici un exemple de classe typique dans la couche logique métier:
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 >>
}Il a beaucoup de duplication, la classe a un certain nombre de responsabilités, etc, etc - c'est juste généralement du code «non nettoyé».
Tout le code du système dépend de mises en œuvre concrètes.
Voici un exemple de classe typique dans la couche d'accès aux données:
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 >>
}Ce système a été développé par moi et une petite équipe en 2005 après un cours .NET d'une semaine. Avant, mon expérience était dans les applications client-serveur. Au cours des 5 dernières années, j'ai fini par reconnaître les avantages des tests unitaires automatisés, des tests d'intégration automatisés et des tests d'acceptation automatisés (en utilisant du sélénium ou équivalent), mais la base de code actuelle semble impossible à introduire ces concepts.
Nous commençons maintenant à travailler sur un projet d'amélioration majeur avec des délais serrés. L'équipe est composée de 5 développeurs .NET - 2 développeurs avec quelques années d'expérience .NET et 3 autres avec peu ou pas d'expérience .NET. Aucune de l'équipe (y compris moi-même) n'a d'expérience dans l'utilisation de frameworks de test unitaire ou de mocking .NET.
Quelle stratégie utiliseriez-vous pour rendre ce code plus propre, plus orienté objet, testable et maintenable?
c#
.net
unit-testing
object-oriented
Anthony
la source
la source
Réponses:
Vous mentionnez deux livres dans lesquels l'un des principaux messages est "The Boy Scout Rule", c'est-à-dire nettoyer le code lorsque vous le touchez. Si vous avez un système qui fonctionne, une réécriture massive est contre-productive. Au lieu de cela, lorsque vous ajoutez de nouvelles fonctionnalités, assurez-vous d'améliorer le code tel qu'il est.
Pour plonger plus loin, Feathers parle de tester l'application à ses coutures: les points logiques auxquels les unités se connectent. Vous pouvez profiter d'une couture pour créer un stub ou une maquette pour une dépendance afin de pouvoir écrire des tests autour de l'objet dépendant. Prenons votre AddressBO comme exemple
Il y a une couture évidente entre l'AdressBO et l'AdressDAO. Créons une interface pour AddressDAO et permettons à la dépendance d'être injectée dans AddressBO.
Maintenant, vous faites le point sur votre AddressBO pour permettre l'injection
Ici, nous utilisons «l'injection de dépendance du pauvre». Notre seul objectif est de briser la couture et de nous permettre de tester l'AdressBO. Maintenant, dans nos tests unitaires, nous pouvons faire un IAddressDAO simulé et valider les interactions entre les deux objets.
la source
Si je me souviens bien travailler efficacement avec le code hérité dit qu'une réécriture complète ne garantit pas que le nouveau code serait meilleur que l'ancien (du point de vue des fonctionnalités / défauts). Les refactorings dans ce livre sont pour la correction de bugs / l'ajout de nouvelles fonctionnalités.
Un autre livre que je recommanderais est Brownfield Application Development in .NET qui dit essentiellement de ne pas faire de réécriture complète également. Il s'agit de faire des changements réguliers et itératifs chaque fois que vous ajoutez de nouvelles fonctionnalités ou corrigez des défauts. Il aborde les considérations de coûts par rapport aux avantages et vous avertit d'essayer de trop mordre à la fois. Alors que Travailler efficacement avec Legacy Code traite principalement de la façon de refactoriser au niveau micro / code, Brownfield Application Development dans .NET , couvre principalement les considérations de niveau supérieur lors de la refactorisation (ainsi que certaines choses au niveau du code aussi).
Le livre Brownfield suggère également de déterminer la zone du code qui vous cause le plus de problèmes et de vous concentrer là-dessus. Tous les autres domaines qui ne nécessitent pas beaucoup d'entretien peuvent ne pas valoir la peine d'être modifiés.
la source
Pour une telle application héritée, il est beaucoup plus rentable de commencer par la couvrir avec des tests d'intégration de niveau supérieur (automatisés) plutôt que des tests unitaires. Ensuite, avec les tests d'intégration comme filet de sécurité, vous pouvez commencer le refactoring par petites étapes si cela est approprié, c'est-à-dire si le coût du refactoring se rembourse à long terme. Comme d'autres l'ont fait remarquer, cela ne va pas de soi.
Voir également ma réponse antérieure à une question similaire; j'espère que vous le trouverez utile.
la source
Soyez très prudent lorsque vous jetez et réécrivez le code en cours d'exécution ( choses que vous ne devriez jamais faire ). Bien sûr, cela peut être moche, mais si cela fonctionne, laissez-le. Voir le blog de Joel, sûr qu'il a plus de 10 ans, mais il est toujours sur la bonne voie.
la source
Comme Mike l'a déclaré, la `` règle du scoutisme '' est probablement la meilleure ici, si le code fonctionne et n'est pas une source constante de rapports de bogues, je préférerais le laisser reposer et l'améliorer lentement au fil du temps.
Au cours de votre projet d'amélioration, prévoyez de nouvelles façons de faire les choses. Par exemple, utilisez un ORM pour les nouvelles fonctionnalités et contournez le modèle de couche de données existant. Lorsque vous rencontrez des améliorations qui doivent toucher le code existant, vous pouvez peut-être déplacer une partie du code associé vers la nouvelle façon de faire. L'utilisation d'une façade ou de certains adaptateurs à certains endroits peut vous aider à isoler l'ancien code, peut-être même par couche. Cela pourrait vous aider à affamer l'ancien code au fil du temps.
De même, cela peut vous aider à ajouter des tests unitaires, vous pouvez commencer avec le nouveau code que vous créez et ajouter lentement des tests pour l'ancien code que vous devez toucher pour les nouvelles améliorations.
la source
Ce sont deux bons livres. Si vous allez commencer à réécrire le code de cette manière, je pense qu'il est important de commencer également à couvrir le code avec des tests unitaires pour l'aider à rester stable pendant que vous le réécrivez.
Cela doit être fait par petites étapes et la modification de ce type de code peut facilement déstabiliser l'ensemble du système.
Je ne modifierais aucun code sur lequel vous ne travaillez pas activement. Ne faites cela que sur du code que vous améliorez ou corrigez activement. Si quelque chose sert son but mais n'a pas été modifié depuis des années, laissez-le simplement. C'est en train de faire ça même si vous connaissez une meilleure façon.
Au bout du compte, l'entreprise a besoin de productivité. Bien qu'un meilleur code augmente la productivité en réécrivant le code simplement parce qu'il pourrait être écrit mieux, ce n'est probablement pas la meilleure façon d'apporter de la valeur à votre produit.
la source