什么是好的代码
我希望能够编写优美的代码。
优美的代码就像一篇散文,易懂易读,而且看起来很漂亮。在《代码之美》一书中,收录了Ruby之父松本行宏的一篇文章,名为《把代码当作文章》,大约表达了同样的含义。Thoughtworks的一位工程师在《软件开发沉思录》一书中提出,每个类的方法最好不要超过5行。最初让我感觉很惊诧,继而觉得不可能。虽然这位工程师言之凿凿,提到在自己参与的项目中,所有代码都完全遵循了这一规范,我仍然表示怀疑。最近,阅读了Robert C. Martin的著作《代码整洁之道》(英文版名为Clean Code),看到Uncle Bob演示的代码,真是漂亮极了。仔细一看,这些好的代码在每个方法中大多数都没有超过5行。诀窍在哪里?那就是重构手法中最常用的Extract Method。进一步讲,如果我们能够为每个类与方法以及变量定义出好的名字,代码确实可以变成一篇散文。当然,是英文散文。
今天,我在重温.NET的序列化时,在MSDN上找到一篇演示Xml序列化的示范代码。或许是因为示范代码的缘故,这一段代码写得极其地不优雅,甚至显得有些丑陋:
public static void Main() {
// Read and write purchase orders.
Test t = new Test();
t.CreatePO("po.xml");
t.ReadPO("po.xml");
}
private void CreatePO(string filename) {
// Create an instance of the XmlSerializer class;
// specify the type of object to serialize.
XmlSerializer serializer =
new XmlSerializer(typeof(PurchaseOrder));
TextWriter writer = new StreamWriter(filename);
PurchaseOrder po = new PurchaseOrder();
// Create an address to ship and bill to.
Address billAddress = new Address();
billAddress.Name = "Teresa Atkinson";
billAddress.Line1 = "1 Main St.";
billAddress.City = "AnyTown";
billAddress.State = "WA";
billAddress.Zip = "00000";
// Set ShipTo and BillTo to the same addressee.
po.ShipTo = billAddress;
po.OrderDate = System.DateTime.Now.ToLongDateString();
// Create an OrderedItem object.
OrderedItem i1 = new OrderedItem();
i1.ItemName = "Widget S";
i1.Description = "Small widget";
i1.UnitPrice = (decimal)5.23;
i1.Quantity = 3;
i1.Calculate();
// Insert the item into the array.
OrderedItem[] items = { i1 };
po.OrderedItems = items;
// Calculate the total cost.
decimal subTotal = new decimal();
foreach (OrderedItem oi in items) {
subTotal += oi.LineTotal;
}
po.SubTotal = subTotal;
po.ShipCost = (decimal)12.51;
po.TotalCost = po.SubTotal + po.ShipCost;
// Serialize the purchase order, and close the TextWriter.
serializer.Serialize(writer, po);
writer.Close();
}
protected void ReadPO(string filename) {
// Create an instance of the XmlSerializer class;
// specify the type of object to be deserialized.
XmlSerializer serializer = new XmlSerializer(typeof(PurchaseOrder));
/* If the XML document has been altered with unknown
nodes or attributes, handle them with the
UnknownNode and UnknownAttribute events.*/
serializer.UnknownNode += new
XmlNodeEventHandler(serializer_UnknownNode);
serializer.UnknownAttribute += new
XmlAttributeEventHandler(serializer_UnknownAttribute);
// A FileStream is needed to read the XML document.
FileStream fs = new FileStream(filename, FileMode.Open);
// Declare an object variable of the type to be deserialized.
PurchaseOrder po;
/* Use the Deserialize method to restore the object's state with
data from the XML document. */
po = (PurchaseOrder)serializer.Deserialize(fs);
// Read the order date.
Console.WriteLine("OrderDate: " + po.OrderDate);
// Read the shipping address.
Address shipTo = po.ShipTo;
ReadAddress(shipTo, "Ship To:");
// Read the list of ordered items.
OrderedItem[] items = po.OrderedItems;
Console.WriteLine("Items to be shipped:");
foreach (OrderedItem oi in items) {
Console.WriteLine("\t" +
oi.ItemName + "\t" +
oi.Description + "\t" +
oi.UnitPrice + "\t" +
oi.Quantity + "\t" +
oi.LineTotal);
}
// Read the subtotal, shipping cost, and total cost.
Console.WriteLine("\t\t\t\t\t Subtotal\t" + po.SubTotal);
Console.WriteLine("\t\t\t\t\t Shipping\t" + po.ShipCost);
Console.WriteLine("\t\t\t\t\t Total\t\t" + po.TotalCost);
}
protected void ReadAddress(Address a, string label) {
// Read the fields of the Address object.
Console.WriteLine(label);
Console.WriteLine("\t" + a.Name);
Console.WriteLine("\t" + a.Line1);
Console.WriteLine("\t" + a.City);
Console.WriteLine("\t" + a.State);
Console.WriteLine("\t" + a.Zip);
Console.WriteLine();
}
private void serializer_UnknownNode
(object sender, XmlNodeEventArgs e) {
Console.WriteLine("Unknown Node:" + e.Name + "\t" + e.Text);
}
private void serializer_UnknownAttribute
(object sender, XmlAttributeEventArgs e) {
System.Xml.XmlAttribute attr = e.Attr;
Console.WriteLine("Unknown attribute " +
attr.Name + "='" + attr.Value + "'");
}
}
看看CreatePO()和ReadPO(),多么地冗长。虽然这个实现极为简单,但对于代码的阅读者而言,想要一下子抓住该方法的中心思想,仍然比较困难。此外,方法中的注释也显得多余,因为,代码本身就可以给予很好的说明。
下面,是我对这段代码的重构,大家可以对比对比,是否更加容易阅读呢?
public static void CreatePurchaseOrder(string filename) {
PurchaseOrder po = BuildPurchaseOrder();
XmlSerializer serializer = new XmlSerializer(typeof(PurchaseOrder));
using (var writer = new StreamWriter(filename)) {
serializer.Serialize(writer, po);
}
}
private static PurchaseOrder BuildPurchaseOrder() {
Address address = CreateAddress();
OrderedItem i1 = CreateOrderedItem();
OrderedItem[] items = { i1 };
PurchaseOrder po = new PurchaseOrder();
po.ShipTo = address;
po.OrderDate = System.DateTime.Now.ToLongDateString();
po.OrderedItems = items;
po.SubTotal = CalculateSubTotal(items);
po.ShipCost = (decimal)12.51;
po.TotalCost = po.SubTotal + po.ShipCost;
return po;
}
private static decimal CalculateSubTotal(OrderedItem[] items) {
decimal subTotal = new decimal();
foreach (OrderedItem oi in items) {
subTotal += oi.LineTotal;
}
return subTotal;
}
private static OrderedItem CreateOrderedItem() {
OrderedItem i1 = new OrderedItem();
i1.ItemName = "Widget S";
i1.Description = "Small widget";
i1.UnitPrice = (decimal)5.23;
i1.Quantity = 3;
i1.Calculate();
return i1;
}
private static Address CreateAddress() {
Address billAddress = new Address();
billAddress.Name = "Bruce Zhang";
billAddress.Line1 = "1 Main St.";
billAddress.City = "Chong Qing";
billAddress.State = "Chong Qing";
billAddress.Zip = "400000";
return billAddress;
}
public static void ReadPurchaseOrder(string filename) {
XmlSerializer serializer = new XmlSerializer(typeof(PurchaseOrder));
serializer.UnknownNode += new XmlNodeEventHandler(serializer_UnknownNode);
serializer.UnknownAttribute += new XmlAttributeEventHandler(serializer_UnknownAttribute);
FileStream fs = new FileStream(filename, FileMode.Open);
PurchaseOrder po;
po = (PurchaseOrder)serializer.Deserialize(fs);
PurchaseOrderPrinter.PrintPurchaseOrder(po);
}
private static void serializer_UnknownNode
(object sender, XmlNodeEventArgs e) {
Console.WriteLine("Unknown Node:" + e.Name + "\t" + e.Text);
}
private static void serializer_UnknownAttribute
(object sender, XmlAttributeEventArgs e) {
System.Xml.XmlAttribute attr = e.Attr;
Console.WriteLine("Unknown attribute " +
attr.Name + "='" + attr.Value + "'");
}
private static class PurchaseOrderPrinter {
public static void PrintPurchaseOrder(PurchaseOrder po) {
PrintOrderDate(po);
PrintAddress(po.ShipTo);
PrintOrderedItem(po.OrderedItems);
PrintOrderCost(po);
}
private static void PrintOrderCost(PurchaseOrder po) {
Console.WriteLine("\t\t\t\t\t Subtotal\t" + po.SubTotal);
Console.WriteLine("\t\t\t\t\t Shipping\t" + po.ShipCost);
Console.WriteLine("\t\t\t\t\t Total\t\t" + po.TotalCost);
}
private static void PrintOrderDate(PurchaseOrder po) {
Console.WriteLine("OrderDate: " + po.OrderDate);
}
private static void PrintOrderedItem(OrderedItem[] items) {
Console.WriteLine("Items to be shipped:");
foreach (OrderedItem oi in items) {
Console.WriteLine("\t" +
oi.ItemName + "\t" +
oi.Description + "\t" +
oi.UnitPrice + "\t" +
oi.Quantity + "\t" +
oi.LineTotal);
}
}
private static void PrintAddress(Address a) {
// Read the fields of the Address object.
Console.WriteLine("Ship To:");
Console.WriteLine("\t" + a.Name);
Console.WriteLine("\t" + a.Line1);
Console.WriteLine("\t" + a.City);
Console.WriteLine("\t" + a.State);
Console.WriteLine("\t" + a.Zip);
Console.WriteLine();
}
}
}
阅读代码时,我们可以先关注最主要的方法,即CreatePurchaseOrder()和ReadPurchaseOrder()方法。如果并不希望了解过多构造PO对象的细节,通过阅读这样简短的方法,可以很容易地抓住这两个方法的实现,那就是通过构建一个PO对象,进行序列化,而在反序列化时,将获得的PO对象信息打印出来。
其实,糟糕的代码不一定就是初学者的“专利”,让我们看看NHibernate中的一段代码:
{
Init();
log.Info("building session factory");
properties = new Dictionary<string, string>(cfg.Properties);
interceptor = cfg.Interceptor;
this.settings = settings;
sqlFunctionRegistry = new SQLFunctionRegistry(settings.Dialect, cfg.SqlFunctions);
eventListeners = listeners;
filters = new Dictionary<string, FilterDefinition>(cfg.FilterDefinitions);
if (log.IsDebugEnabled)
{
log.Debug("Session factory constructed with filter configurations : " + CollectionPrinter.ToString(filters));
}
if (log.IsDebugEnabled)
{
log.Debug("instantiating session factory with properties: " + CollectionPrinter.ToString(properties));
}
try
{
if (settings.IsKeywordsImportEnabled)
{
SchemaMetadataUpdater.Update(this);
}
if (settings.IsAutoQuoteEnabled)
{
SchemaMetadataUpdater.QuoteTableAndColumns(cfg);
}
}
catch (NotSupportedException)
{
// Ignore if the Dialect does not provide DataBaseSchema
}
#region Caches
settings.CacheProvider.Start(properties);
#endregion
#region Generators
identifierGenerators = new Dictionary<string, IIdentifierGenerator>();
foreach (PersistentClass model in cfg.ClassMappings)
{
if (!model.IsInherited)
{
IIdentifierGenerator generator =
model.Identifier.CreateIdentifierGenerator(settings.Dialect, settings.DefaultCatalogName,
settings.DefaultSchemaName, (RootClass) model);
identifierGenerators[model.EntityName] = generator;
}
}
#endregion
#region Persisters
Dictionary<string, ICacheConcurrencyStrategy> caches = new Dictionary<string, ICacheConcurrencyStrategy>();
entityPersisters = new Dictionary<string, IEntityPersister>();
implementorToEntityName = new Dictionary<System.Type, string>();
Dictionary<string, IClassMetadata> classMeta = new Dictionary<string, IClassMetadata>();
foreach (PersistentClass model in cfg.ClassMappings)
{
model.PrepareTemporaryTables(mapping, settings.Dialect);
string cacheRegion = model.RootClazz.CacheRegionName;
ICacheConcurrencyStrategy cache;
if (!caches.TryGetValue(cacheRegion, out cache))
{
cache =
CacheFactory.CreateCache(model.CacheConcurrencyStrategy, cacheRegion, model.IsMutable, settings, properties);
if (cache != null)
{
caches.Add(cacheRegion, cache);
allCacheRegions.Add(cache.RegionName, cache.Cache);
}
}
IEntityPersister cp = PersisterFactory.CreateClassPersister(model, cache, this, mapping);
entityPersisters[model.EntityName] = cp;
classMeta[model.EntityName] = cp.ClassMetadata;
if (model.HasPocoRepresentation)
{
implementorToEntityName[model.MappedClass] = model.EntityName;
}
}
classMetadata = new UnmodifiableDictionary<string, IClassMetadata>(classMeta);
Dictionary<string, ISet<string>> tmpEntityToCollectionRoleMap = new Dictionary<string, ISet<string>>();
collectionPersisters = new Dictionary<string, ICollectionPersister>();
foreach (Mapping.Collection model in cfg.CollectionMappings)
{
ICacheConcurrencyStrategy cache =
CacheFactory.CreateCache(model.CacheConcurrencyStrategy, model.CacheRegionName, model.Owner.IsMutable, settings,
properties);
if (cache != null)
{
allCacheRegions[cache.RegionName] = cache.Cache;
}
ICollectionPersister persister = PersisterFactory.CreateCollectionPersister(cfg, model, cache, this);
collectionPersisters[model.Role] = persister;
IType indexType = persister.IndexType;
if (indexType != null && indexType.IsAssociationType && !indexType.IsAnyType)
{
string entityName = ((IAssociationType) indexType).GetAssociatedEntityName(this);
ISet<string> roles;
if (!tmpEntityToCollectionRoleMap.TryGetValue(entityName, out roles))
{
roles = new HashedSet<string>();
tmpEntityToCollectionRoleMap[entityName] = roles;
}
roles.Add(persister.Role);
}
IType elementType = persister.ElementType;
if (elementType.IsAssociationType && !elementType.IsAnyType)
{
string entityName = ((IAssociationType) elementType).GetAssociatedEntityName(this);
ISet<string> roles;
if (!tmpEntityToCollectionRoleMap.TryGetValue(entityName, out roles))
{
roles = new HashedSet<string>();
tmpEntityToCollectionRoleMap[entityName] = roles;
}
roles.Add(persister.Role);
}
}
Dictionary<string, ICollectionMetadata> tmpcollectionMetadata = new Dictionary<string, ICollectionMetadata>(collectionPersisters.Count);
foreach (KeyValuePair<string, ICollectionPersister> collectionPersister in collectionPersisters)
{
tmpcollectionMetadata.Add(collectionPersister.Key, collectionPersister.Value.CollectionMetadata);
}
collectionMetadata = new UnmodifiableDictionary<string, ICollectionMetadata>(tmpcollectionMetadata);
collectionRolesByEntityParticipant = new UnmodifiableDictionary<string, ISet<string>>(tmpEntityToCollectionRoleMap);
#endregion
#region Named Queries
namedQueries = new Dictionary<string, NamedQueryDefinition>(cfg.NamedQueries);
namedSqlQueries = new Dictionary<string, NamedSQLQueryDefinition>(cfg.NamedSQLQueries);
sqlResultSetMappings = new Dictionary<string, ResultSetMappingDefinition>(cfg.SqlResultSetMappings);
#endregion
imports = new Dictionary<string, string>(cfg.Imports);
#region after *all* persisters and named queries are registered
foreach (IEntityPersister persister in entityPersisters.Values)
{
persister.PostInstantiate();
}
foreach (ICollectionPersister persister in collectionPersisters.Values)
{
persister.PostInstantiate();
}
#endregion
#region Serialization info
name = settings.SessionFactoryName;
try
{
uuid = (string) UuidGenerator.Generate(null, null);
}
catch (Exception)
{
throw new AssertionFailure("Could not generate UUID");
}
SessionFactoryObjectFactory.AddInstance(uuid, name, this, properties);
#endregion
log.Debug("Instantiated session factory");
#region Schema management
if (settings.IsAutoCreateSchema)
{
new SchemaExport(cfg).Create(false, true);
}
if ( settings.IsAutoUpdateSchema )
{
new SchemaUpdate(cfg).Execute(false, true);
}
if (settings.IsAutoValidateSchema)
{
new SchemaValidator(cfg, settings).Validate();
}
if (settings.IsAutoDropSchema)
{
schemaExport = new SchemaExport(cfg);
}
#endregion
#region Obtaining TransactionManager
// not ported yet
#endregion
currentSessionContext = BuildCurrentSessionContext();
if (settings.IsQueryCacheEnabled)
{
updateTimestampsCache = new UpdateTimestampsCache(settings, properties);
queryCache = settings.QueryCacheFactory.GetQueryCache(null, updateTimestampsCache, settings, properties);
queryCaches = new ThreadSafeDictionary<string, IQueryCache>(new Dictionary<string, IQueryCache>());
}
else
{
updateTimestampsCache = null;
queryCache = null;
queryCaches = null;
}
#region Checking for named queries
if (settings.IsNamedQueryStartupCheckingEnabled)
{
IDictionary<string, HibernateException> errors = CheckNamedQueries();
if (errors.Count > 0)
{
StringBuilder failingQueries = new StringBuilder("Errors in named queries: ");
foreach (KeyValuePair<string, HibernateException> pair in errors)
{
failingQueries.Append('{').Append(pair.Key).Append('}');
log.Error("Error in named query: " + pair.Key, pair.Value);
}
throw new HibernateException(failingQueries.ToString());
}
}
#endregion
Statistics.IsStatisticsEnabled = settings.IsStatisticsEnabled;
// EntityNotFoundDelegate
IEntityNotFoundDelegate enfd = cfg.EntityNotFoundDelegate;
if (enfd == null)
{
enfd = new DefaultEntityNotFoundDelegate();
}
entityNotFoundDelegate = enfd;
}
这是类SessionFactoryImpl(它实现了ISessionFactoryImplementor接口)的构造函数,其目的时是通过Configuration以及Setting中的某些值,去初始化SessionFactoryImpl,然后构建该类的对象。坦白说,我从来没有看过如此“浩瀚无垠”的构造函数。幸好,Visual Studio提高了Region,否则,更让人头疼。(我在想,既然代码的编写者已经利用了Region来分割实现,为何不进一步将其分割为小的方法呢?)
看这样的代码,我们能够轻易读懂吗?
拙劣代码可谓遗患无穷。在《程序员修炼之道》一书中,提到了所谓“破窗效应”,即“没修复的破窗,导致更多的窗户被打破”。丑陋的代码如果只有一个小的片段,看似无关紧要,就像一幢大楼的一扇破窗一般容易让人忘记。随着时间的推移,当这些丑陋代码不知不觉蔓延到整个项目中时,我们才发现这一幢大楼已经满目疮痍了。“一屋不扫,何以扫天下”,程序员应该从小处着手,未来才可能写出优雅的代码。