Я читал « Эффективная работа с устаревшим кодом» и « Чистый код» с целью изучения стратегий, как начать очистку существующей кодовой базы большого приложения веб-форм ASP.NET.
Эта система существует с 2005 года и с тех пор претерпела ряд усовершенствований. Первоначально код был структурирован следующим образом (и все еще в значительной степени структурирован таким образом):
- ASP.NET (aspx / ascx)
- Код позади (c #)
- Уровень бизнес-логики (c #)
- Уровень доступа к данным (c #)
- База данных (Oracle)
Основная проблема заключается в том, что код процедурно маскируется под объектно-ориентированный. Это фактически нарушает все рекомендации, описанные в обеих книгах.
Это пример типичного класса на уровне бизнес-логики:
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 >>
}У него много дублирования, у класса есть ряд обязанностей и т. Д. И т. Д. - это просто «неочищенный» код.
Весь код во всей системе зависит от конкретных реализаций.
Это пример типичного класса на уровне доступа к данным:
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 >>
}Эта система была разработана мной и небольшой командой еще в 2005 году после 1-недельного курса .NET. До этого мой опыт был в клиент-серверных приложениях. За последние 5 лет я осознал преимущества автоматизированного модульного тестирования, автоматизированного интеграционного тестирования и автоматического приемочного тестирования (с использованием Selenium или аналогичного), но текущая кодовая база кажется невозможной для введения этих концепций.
Сейчас мы начинаем работать над крупным проектом по улучшению в сжатые сроки. Команда состоит из 5 разработчиков .NET - 2 разработчика с опытом работы в .NET несколько лет и 3 других с небольшим опытом работы .NET или вообще без него. Никто из команды (включая меня) не имеет опыта в использовании модульного тестирования .NET или макетов фреймворков.
Какую стратегию вы бы использовали, чтобы сделать этот код чище, более объектно-ориентированным, тестируемым и обслуживаемым?
c#
.net
unit-testing
object-oriented
Энтони
источник
источник
Ответы:
Вы упомянули две книги, в которых одно из основных сообщений - «Правило бойскаута», т.е. очищайте код по мере его прикосновения. Если у вас есть работающая система, массовое переписывание неэффективно. Вместо этого, по мере добавления новой функциональности, убедитесь, что вы улучшаете код в том виде, в каком он есть.
Чтобы углубиться в подробности, Feathers рассказывает о тестировании приложения по его швам: логические точки, в которых соединяются устройства. Вы можете использовать шов для создания заглушки или макета для зависимости, чтобы вы могли писать тесты вокруг зависимого объекта. Давайте возьмем ваш AddressBO в качестве примера
Существует очевидный шов между AddressBO и AddressDAO. Давайте создадим интерфейс для AddressDAO и позволим вводить зависимость в AddressBO.
Теперь вы выписываете адрес AddressBO для инъекций
Здесь мы используем «инъекцию зависимости бедного человека». Наша единственная цель - сломать шов и позволить нам протестировать AddressBO. Теперь в наших модульных тестах мы можем создать макет IAddressDAO и проверить взаимодействие между двумя объектами.
источник
Если я правильно помню Эффективная работа с устаревшим кодом» говорит, что полное переписывание не гарантирует, что новый код будет лучше старого (с точки зрения функциональности / дефектов). Рефакторинг в этой книге предназначен для исправления ошибок и добавления новых функций.
Еще одна книга, которую я бы порекомендовал, - это Разработка приложений Brownfield в .NET, в которой, в основном, говорится, что не нужно полностью переписывать. В нем говорится о постоянных итеративных изменениях, когда вы добавляете новые функции или исправляете дефекты. Он учитывает соотношение затрат и выгод и предупреждает о попытках откусить слишком много за один раз. В то время как Эффективная Работа с Устаревшим Кодом говорит в основном о том, как провести рефакторинг на микро / кодовом уровне, Разработка приложений Brownfield в .NET , в основном, охватывает аспекты более высокого уровня при ре-факторинге (наряду с некоторыми вещами на уровне кода).
Книга Браунфилда также предлагает выяснить, какая часть кода доставляет вам больше всего хлопот, и сосредоточиться на ней. Любые другие области, которые не требуют большого обслуживания, могут не стоить менять.
источник
Для такого унаследованного приложения гораздо выгоднее начать с покрытия его (автоматизированными) интеграционными тестами более высокого уровня, а не модульными тестами. Затем, используя интеграционные тесты в качестве защитной сети, вы можете начать рефакторинг небольшими шагами, если это необходимо, т.е. если затраты на рефакторинг окупаются в течение длительного времени. Как отметили другие, это не самоочевидно.
Смотрите также этот мой предыдущий ответ на аналогичный вопрос; Надеюсь, что вы найдете ее полезной.
источник
Будьте очень осторожны, выбрасывая и переписывая исполняемый код (то, что вы никогда не должны делать ). Конечно, это может быть некрасиво, но если это работает, оставьте это. Посмотрите сообщение в блоге Джоэла, конечно, ему более 10 лет, но оно все еще точно в цель.
источник
Как заявил Майк, «правило бойскаута» здесь, вероятно, лучше всего, если код работает и не является постоянным источником сообщений об ошибках, я бы предпочел оставить его там и постепенно улучшать его с течением времени.
Во время вашего проекта улучшения учтите новые способы ведения дел. Например, используйте ORM для новых функций и обходите существующий шаблон уровня данных. Когда вы сталкиваетесь с улучшениями, которые должны касаться существующего кода, вы можете переместить часть связанного кода на новый способ работы. Использование фасада или некоторых адаптеров на местах может помочь вам изолировать старый код, возможно, даже для каждого слоя. Это может помочь вам голодать старый код во времени.
Точно так же это может помочь вам с добавлением модульных тестов, вы можете начать с нового кода, который вы делаете, и медленно добавлять некоторые тесты для старого кода, к которому нужно прикасаться для новых улучшений.
источник
Это обе хорошие книги. Если вы собираетесь начать переписывать код таким образом, я думаю, что важно также начать покрывать код модульными тестами, чтобы помочь сохранить его стабильность при переписывании.
Это должно быть сделано небольшими шагами, и изменение такого рода кода может легко дестабилизировать всю систему.
Я бы не стал изменять код, над которым вы активно не работаете. Делайте это только с кодом, который вы активно улучшаете или исправляете. Если что-то служит своей цели, но не изменялось годами, просто оставьте это. Он делает свое дело, даже если вы знаете лучший способ.
В конце дня компании нужна производительность. Хотя лучший код повышает производительность, переписывание кода только потому, что его можно было написать лучше, вероятно, не лучший способ повысить ценность вашего продукта.
источник