什么是好的代码

简介:

我希望能够编写优美的代码。

优美的代码就像一篇散文,易懂易读,而且看起来很漂亮。在《代码之美》一书中,收录了Ruby之父松本行宏的一篇文章,名为《把代码当作文章》,大约表达了同样的含义。Thoughtworks的一位工程师在《软件开发沉思录》一书中提出,每个类的方法最好不要超过5行。最初让我感觉很惊诧,继而觉得不可能。虽然这位工程师言之凿凿,提到在自己参与的项目中,所有代码都完全遵循了这一规范,我仍然表示怀疑。最近,阅读了Robert C. Martin的著作《代码整洁之道》(英文版名为Clean Code),看到Uncle Bob演示的代码,真是漂亮极了。仔细一看,这些好的代码在每个方法中大多数都没有超过5行。诀窍在哪里?那就是重构手法中最常用的Extract Method。进一步讲,如果我们能够为每个类与方法以及变量定义出好的名字,代码确实可以变成一篇散文。当然,是英文散文。

今天,我在重温.NET的序列化时,在MSDN上找到一篇演示Xml序列化的示范代码。或许是因为示范代码的缘故,这一段代码写得极其地不优雅,甚至显得有些丑陋:

public  class  Test {
???? 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  class  PurchaseOrderHandler {
???????? 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中的一段代码:

public SessionFactoryImpl( Configuration cfg,  IMapping mapping,  Settings settings,  EventListeners listeners)
{
????Init();
????log.Info( "building session factory");

????properties =  new  Dictionary< stringstring>(cfg.Properties);
????interceptor = cfg.Interceptor;
???? this.settings = settings;
????sqlFunctionRegistry =  new  SQLFunctionRegistry(settings.Dialect, cfg.SqlFunctions);
????eventListeners = listeners;
????filters =  new  Dictionary< stringFilterDefinition>(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< stringIIdentifierGenerator>();
???? 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< stringICacheConcurrencyStrategy> caches =  new  Dictionary< stringICacheConcurrencyStrategy>();
????entityPersisters =  new  Dictionary< stringIEntityPersister>();
????implementorToEntityName =  new  Dictionary<System. Typestring>();

???? Dictionary< stringIClassMetadata> classMeta =  new  Dictionary< stringIClassMetadata>();

???? 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< stringIClassMetadata>(classMeta);

???? Dictionary< stringISet&lt; string>&gt; tmpEntityToCollectionRoleMap =  new  Dictionary< stringISet&lt; string>&gt;();
????collectionPersisters =  new  Dictionary< stringICollectionPersister>();
???? 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< stringICollectionMetadata> tmpcollectionMetadata =  new  Dictionary< stringICollectionMetadata>(collectionPersisters.Count);
???? foreach ( KeyValuePair< stringICollectionPersister> collectionPersister  in collectionPersisters)
????{
????????tmpcollectionMetadata.Add(collectionPersister.Key, collectionPersister.Value.CollectionMetadata);
????}
????collectionMetadata =  new  UnmodifiableDictionary< stringICollectionMetadata>(tmpcollectionMetadata);
????collectionRolesByEntityParticipant =  new  UnmodifiableDictionary< stringISet&lt; string>&gt;(tmpEntityToCollectionRoleMap);
???? #endregion

???? #region Named Queries
????namedQueries =  new  Dictionary< stringNamedQueryDefinition>(cfg.NamedQueries);
????namedSqlQueries =  new  Dictionary< stringNamedSQLQueryDefinition>(cfg.NamedSQLQueries);
????sqlResultSetMappings =  new  Dictionary< stringResultSetMappingDefinition>(cfg.SqlResultSetMappings);
???? #endregion

????imports =  new  Dictionary< stringstring>(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( nullnull);
????}
???? 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( falsetrue);
????}

???? if ( settings.IsAutoUpdateSchema )
????{
???????? new  SchemaUpdate(cfg).Execute( falsetrue);
????}
???? 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< stringIQueryCache>( new  Dictionary< stringIQueryCache>());
????}
???? else
????{
????????updateTimestampsCache =  null;
????????queryCache =  null;
????????queryCaches =  null;
????}

???? #region Checking for named queries
???? if (settings.IsNamedQueryStartupCheckingEnabled)
????{
???????? IDictionary< stringHibernateException> errors = CheckNamedQueries();
???????? if (errors.Count &gt; 0)
????????{
???????????? StringBuilder failingQueries =  new  StringBuilder( "Errors in named queries: ");
???????????? foreach ( KeyValuePair< stringHibernateException> pair  in errors)
????????????{
????????????????failingQueries.Append( &#39;{&#39;).Append(pair.Key).Append( &#39;}&#39;);
????????????????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来分割实现,为何不进一步将其分割为小的方法呢?)

看这样的代码,我们能够轻易读懂吗?

拙劣代码可谓遗患无穷。在《程序员修炼之道》一书中,提到了所谓“破窗效应”,即“没修复的破窗,导致更多的窗户被打破”。丑陋的代码如果只有一个小的片段,看似无关紧要,就像一幢大楼的一扇破窗一般容易让人忘记。随着时间的推移,当这些丑陋代码不知不觉蔓延到整个项目中时,我们才发现这一幢大楼已经满目疮痍了。“一屋不扫,何以扫天下”,程序员应该从小处着手,未来才可能写出优雅的代码。








本文转自wayfarer51CTO博客,原文链接:http://blog.51cto.com/wayfarer/310016,如需转载请自行联系原作者

相关文章
|
Java Android开发
几行代码就能实现为何要多此一举
几行代码就能搞定,不能代表一个人很牛,借助了开源,只是站在了巨人的肩膀上,让你省去了去往成功的一大段路,然而这一段路上的风景,还请你仔细去欣赏,到头来,你会发现,路上的风景会远远美于终点的成功。
|
Java Python
长见识,让大家看看什么是垃圾代码
长见识,让大家看看什么是垃圾代码
119 0
|
定位技术 数据处理 开发工具
如何优雅地统计代码(一)
*精美排版详见钉钉文档其实这个事情要从一个下午讲起,对我来说是个尤里卡时刻;其实一开始让我直接从数据里统计大家提交代码是有点无从下手的,前几天开始调研了一波代码统计方案后发现大部分都是基于文件来统计代码的各种行数并没有这种基于前后版本的变更代码统计,大家更多的使用Git自带的统计方法但显然我这里没有这样的环境(下面背景会详细展开),快要放弃今天的技术调研遂下楼散步刷新思维,我又回溯了我在这个项目中
最近特火的爱心代码来了
最近因为一部《点燃我温暖你》的电视剧而爆火的爱心代码不会还有人不会制作吧。
最近特火的爱心代码来了
李峋的爱心代码
《点燃我温暖你》中李峋的爱心代码
193 0
|
设计模式 IDE Java
如何将代码写的更加优雅
如何将代码写的更加优雅
|
前端开发
代码为什么越写越乱?
这个问题往大的说是业务治理问题,往小了说是代码分拆。且看作者怎么写出好代码。
152 0
|
Web App开发 安全 编译器
如何保护你的代码 - Ollvm(一)
如何保护你的代码 - Ollvm(一)
如何保护你的代码 - Ollvm(一)