BORING - C# - Would you rate the readability of my code?
-
This is probably the most boring request ever, but I have some code that I would like to have read over, and assessed for its readability and the readability of its comments.
Since I am a self-taught programmer (all the way from QBASIC!) i really would like some "real-world" programmers to show me how my code can be better commented.This is for a vector image format I am calling "Constructive Planar Geometry" or CPG for short.
The files are XML such like:
http://67.67.35.17/biohazard.cpgI left off all of the object primitive classes, because they are strctly internal.
However, if you want the full source, it is available here:
http://67.67.35.17/ImageCPG.cs#region Copyright John Gietzen 2005
//
// All rights are reserved. Reproduction or transmission in whole or in part, in
// any form or by any means, electronic, mechanical or otherwise, is prohibited
// without the prior written consent of the copyright owner.
//
#endregion
using System;
using System.ComponentModel;
using System.Drawing;
using System.IO;
using System.Windows.Forms;
using System.Xml;
namespace Phidias
{
/// <summary>
/// Represents a CPG Image.
/// </summary>
public class ImageCPG : UserControl
{
private CPG image = null;
private Timer ResizeTimer;
private Image pic = null;
/// <summary>
/// Initializes a new instance of the ImageCPG class.
/// </summary>
public ImageCPG()
{
InitializeComponent();
}
/// <summary>
/// Gets or sets the filename of the image.
/// </summary>
public string File
{
get { return _File; }
set { LoadImage( value ); }
}
private string _File = "";
/// <summary>
/// Loads an image from the specified filename.
/// </summary>
/// <param name="Filename">The complete filename to read.</param>
public void LoadImage(string Filename)
{
_File = Filename;
pic = null;
string xml = "";
try
{
TextReader reader = new StreamReader( _File );
xml = reader.ReadToEnd();
reader.Close();
}
catch
{
return;
}
image = new CPG( xml );
Render();
}
/// <summary>
/// Forces an immediate render of the image.
/// </summary>
public void Render()
{
if( image != null )
{
if( pic == null )
{
pic = image.Render( Width, Height, BackColor );
Invalidate();
}
else if( Width > pic.Width || Height > pic.Height )
{
pic = image.Render( Math.Max( pic.Width, Width ), Math.Max( pic.Height, Height ), BackColor );
Invalidate();
}
}
}
#region Component Designer generated code
private IContainer components;
/// <summary>
/// Clean up any resources being used.
/// </summary>
protected override void Dispose(bool disposing)
{
if( disposing )
{
if( components != null )
{
components.Dispose();
}
}
base.Dispose( disposing );
}
/// <summary>
/// Required method for Designer support - do not modify
/// the contents of this method with the code editor.
/// </summary>
private void InitializeComponent()
{
components = new Container();
ResizeTimer = new Timer( components );
//
// ResizeTimer
//
ResizeTimer.Interval = 2000;
ResizeTimer.Tick += new EventHandler( ResizeTimer_Tick );
//
// ImageCPG
//
Name = "ImageCPG";
Size = new Size( 128, 128 );
Resize += new EventHandler( ImageCPG_Resize );
Paint += new PaintEventHandler( ImageCPG_Paint );
}
#endregion
private void ImageCPG_Paint(object sender, PaintEventArgs e)
{
if( pic != null )
{
e.Graphics.DrawImage( pic, 0, 0, Width, Height );
}
}
private void ResizeTimer_Tick(object sender, EventArgs e)
{
ResizeTimer.Enabled = false;
Render();
}
private void ImageCPG_Resize(object sender, EventArgs e)
{
ResizeTimer.Enabled = false;
ResizeTimer.Enabled = true;
}
}
/// <summary>
/// Wrapper class for loading and rendering CPG objects.
/// </summary>
class CPG
{
private double ScaleLeft, ScaleTop, ScaleWidth, ScaleHeight;
private Color Background = Color.White;
private CPGObject root = null;
/// <summary>
/// Constructs a new CPG Image based on an XML string.
/// </summary>
/// <param name="XML">The XML content to convert.</param>
public CPG(string XML)
{
try
{
// Convert content to an XML Document
XmlDocument doc = new XmlDocument();
doc.LoadXml( XML );
// Convert the XML Document to CPG
ReadXML( doc.FirstChild.ParentNode );
}
catch
{
root = null;
}
}
/// <summary>
/// Constructs a new CPG Image based on an XML Document.
/// </summary>
/// <param name="Document">The XML document to convert.</param>
public CPG(XmlDocument Document)
{
try
{
// Convert the XML Document to CPG
ReadXML( Document.FirstChild.ParentNode );
}
catch
{
root = null;
}
}
/// <summary>
/// Renders the CPG Image to a System.Drawing.Bitmap object.
/// </summary>
/// <param name="Width">The width, in pixels, of the new Bitmap object.</param>
/// <param name="Height">The height, in pixels, of the new Bitmap object.</param>
/// <param name="BGColor">The background color to be mixed under the image.</param>
/// <returns>A System.Drawing.Bitmap object representing the CPG Image</returns>
public Bitmap Render(int Width, int Height, Color BGColor)
{
if( root == null )
{
return null;
}
Color pixel;
Bitmap BMP = new Bitmap( Width, Height );
for( int y = 0; y < Height; y++ )
{
for( int x = 0; x < Width; x++ )
{
// Reset the background color
Color basecolor = CPGColorSet.Mix( Background, BGColor );
// Calculate our position in Image-Space
double ScaleX = ScaleLeft + (ScaleWidth * x) / Width;
double ScaleY = ScaleTop + (ScaleHeight * y) / Height;
// Trace the pixel
pixel = root.Shade( ScaleX, ScaleY, basecolor );
BMP.SetPixel( x, y, pixel );
}
}
return BMP;
}
/// <summary>
/// Parses an XML Node into the root CPG object.
/// </summary>
/// <param name="node"></param>
private void ReadXML(XmlNode node)
{
try
{
// Reset all file-based variables
root = null;
defines = new CPGObject[2048];
indices = new string[2048];
Background = Color.FromArgb( 0, 0, 0, 0 );
ScaleHeight = ScaleWidth = 100.0f;
ScaleLeft = ScaleTop = 0.0f;
if( node == null )
{
return;
}
// If we are in the document node, search for a node named "cpg"
// This is necessary in the case that a document has an XML or a DOCTYPE declaration
if( node.Name == "#document" )
{
node = node.FirstChild;
while( node.Name != "cpg" )
{
node = node.NextSibling;
// If all nodes are exhausted before a match is found, exit.
if( node == null )
{
return;
}
}
}
// If we are indeed in the "cpg" node, set the image-wide defaults
if( node.Name == "cpg" )
{
foreach( XmlAttribute attrib in node.Attributes )
{
switch( attrib.Name )
{
case "left":
ScaleLeft = Convert.ToSingle( attrib.Value );
break;
case "top":
ScaleTop = Convert.ToSingle( attrib.Value );
break;
case "width":
ScaleWidth = Convert.ToSingle( attrib.Value );
break;
case "height":
ScaleHeight = Convert.ToSingle( attrib.Value );
break;
case "background":
Background = CPGColorSet.CalcColor( attrib.Value );
break;
}
}
}
// Copy the readonly default color set
CPGColorSet BaseColorSet = new CPGColorSet( CPGColorSet.Default );
// Convert the document
root = NodeToCPGObject( node, BaseColorSet );
}
catch
{
return;
}
}
private CPGObject[] defines = null;
private string[] indices = null;
private enum CPGGroupType
{
None,
Union,
Merge,
Intersection,
Difference,
Inverse
} ;
private CPGObject NodeToCPGObject(XmlNode node, CPGColorSet color)
{
if( node == null )
{
return new CPGNothing();
}
CPGGroupType group = CPGGroupType.Union;
CPGColorSet basecolor = new CPGColorSet( color );
#region Boring Attribute-to-Variable Variables...
double
x = 0.0f,
y = 0.0f,
width = 0.0f,
height = 0.0f,
r = 0.0f,
rx = 0.0f,
ry = 0.0f;
string name = "";
#endregion
foreach( XmlAttribute attrib in node.Attributes )
{
switch( attrib.Name )
{
case "fill":
basecolor.Fill = CPGColorSet.CalcColor( attrib.Value );
break;
case "line":
basecolor.Line = CPGColorSet.CalcColor( attrib.Value );
break;
case "border":
basecolor.Thickness = Convert.ToSingle( attrib.Value );
break;
#region ... Attribute-to-Variable Conversions
case "name":
name = attrib.Value;
break;
case "x":
x = Convert.ToSingle( attrib.Value );
break;
case "y":
y = Convert.ToSingle( attrib.Value );
break;
case "angle":
case "r":
r = Convert.ToSingle( attrib.Value );
break;
case "rx":
rx = Convert.ToSingle( attrib.Value );
break;
case "ry":
ry = Convert.ToSingle( attrib.Value );
break;
case "width":
width = Convert.ToSingle( attrib.Value );
break;
case "height":
height = Convert.ToSingle( attrib.Value );
break;
#endregion
}
}
// Count the children nodes to this node.
int ChildCount = 0;
if( node.ChildNodes != null )
{
ChildCount = node.ChildNodes.Count;
}
#region Initialize the Group Type
switch( node.Name )
{
case "cpg":
case "union":
group = CPGGroupType.Union;
break;
case "merge":
group = CPGGroupType.Merge;
break;
case "inter":
case "intersection":
group = CPGGroupType.Intersection;
break;
case "diff":
case "difference":
group = CPGGroupType.Difference;
break;
case "inv":
case "inverse":
group = CPGGroupType.Inverse;
break;
default:
group = CPGGroupType.None;
break;
}
#endregion
switch( node.Name )
{
case "cpg":
case "union":
case "merge":
case "inter":
case "intersection":
case "diff":
case "difference":
case "inv":
case "inverse":
if( ChildCount == 0 )
{
return new CPGNothing();
}
else if( ChildCount == 1 )
{
if( group == CPGGroupType.Inverse )
{
basecolor.Thickness = -basecolor.Thickness;
return new CPGInverse( NodeToCPGObject( node.FirstChild, basecolor ), basecolor );
}
return NodeToCPGObject( node.FirstChild, basecolor );
}
else
{
CPGObject[] children = new CPGObject[ChildCount];
node = node.FirstChild;
if( group == CPGGroupType.Difference || group == CPGGroupType.Inverse )
{
int i = 0;
if( group == CPGGroupType.Difference )
{
children[0] = NodeToCPGObject( node, basecolor );
i = 1;
}
basecolor.Thickness = -basecolor.Thickness;
for( ; i < ChildCount; i++ )
{
node = node.NextSibling;
children[i] = new CPGInverse( NodeToCPGObject( node, basecolor ), basecolor );
}
}
else
{
for( int i = 0; i < ChildCount; i++ )
{
children[i] = NodeToCPGObject( node, basecolor );
node = node.NextSibling;
}
}
switch( group )
{
case CPGGroupType.Merge:
return new CPGMerge( children, basecolor );
case CPGGroupType.Intersection:
case CPGGroupType.Inverse:
case CPGGroupType.Difference:
return new CPGIntersection( children, basecolor );
case CPGGroupType.Union:
default:
return new CPGUnion( children, basecolor );
}
}
case "define":
for( int i = 0; i < indices.Length; i++ )
{
if( indices[i] == name || defines[i] == null )
{
indices[i] = name;
if( ChildCount == 0 )
{
defines[i] = null;
break;
}
else
{
CPGObject[] children = new CPGObject[ChildCount];
node = node.FirstChild;
for( int j = 0; j < ChildCount; j++ )
{
children[j] = NodeToCPGObject( node, basecolor );
node = node.NextSibling;
}
defines[i] = new CPGUnion( children, basecolor );
break;
}
}
}
return new CPGNothing();
case "recall":
for( int i = 0; i < indices.Length; i++ )
{
if( defines[i] == null )
{
break;
}
if( indices[i] == name )
{
return defines[i];
}
}
return new CPGNothing();
case "circ":
case "circle":
return new CPGCircle( x, y, r, basecolor );
case "ellipse":
return new CPGEllipse( x, y, rx, ry, basecolor );
case "rect":
case "rectangle":
return new CPGRectangle( x, y, width, height, basecolor );
case "rotate":
if( ChildCount == 0 )
{
return new CPGNothing();
}
else
{
CPGObject[] children = new CPGObject[ChildCount];
node = node.FirstChild;
for( int i = 0; i < ChildCount; i++ )
{
children[i] = NodeToCPGObject( node, basecolor );
node = node.NextSibling;
}
return new CPGRotate( new CPGUnion( children, basecolor ), r, x, y, basecolor );
}
default:
return new CPGNothing();
}
}
}
}
-
A few things on design:
- Separate the parsing from the processing. This will give you a
few advantages: First, you'll decouple your particular storage format
from your classes, allowing you to switch formats as the need
arises. Second, you'll be able to unit test your classes by
manipulating objects and properties instead of by constructing bulky
XML documents. Finally, it's more maintainable because the class
has a more specific purpose.
- In .Net, I'd recommend creating a basic object model and using
built-in XML serialization to deserialize your configuration to this
model. It's a lot less error-prone, and can easily support moving
to different XML formats (such as using elements instead of attributes
for certain types of data.) It's also easier to test.
- What you have here is well-suited to the Builder pattern. I'd
suggest writing a "CPGBuilder" class or set of classes which you can
configure to pump out the appropriate objects through code.
On implementation:
- The NodeToCPGObject method can really stand to be broken into smaller pieces, especially the long switch/case section.
- You have an empty 'catch' bock in your ReadXML and LoadImage methods
- ack! You shouldn't ever ignore errors like that. If you
leave that in, I guaruntee one day you'll be scratching your head
looking at a half-constructed object, wondering what happened...
- Use your naming conventions consistently. As member variables
you have '_File', 'ResizeTimer', 'image', and 'pic.' I'd
recommend underscores and camel case, so that would be _file,
_resizeTimer, _image, and _pic.
That's about all for now =)
- Separate the parsing from the processing. This will give you a
-
If you can understand what it does several months after you've left it alone, then you can safely say your code is readable enough.
-
Thanks a million, I have only been using C# for a month or so now, so I dont know what a Builder pattern is. Is that an abstract class, or is that a design type?
Also, this is a very un-finished project, so that empty catch block is only temporary.
And finally, do you have any links for use of the built-in XML serialization? The method I used was based on all the information I could dig-up.
Thanks for your help,
John "otac0n" Gietzen
-
@Otac0n said:
Thanks a million, I have only been using C# for a month or so now, so I dont know what a Builder pattern is. Is that an abstract class, or is that a design type?
Neither, it's a software
-
Take a look at the MSDN documentation on the XmlSerializer class at
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/cpref/html/frlrfsystemxmlserializationxmlserializerclasstopic.asp.
The code to make it work is extremely simple - so simple in fact that I
rarely see the need to iterate through XmlDocuments any more at all.
-
Take this:
if( disposing ){
if( components != null )
{
components.Dispose();
}
}
You are advised to compact that to:
if (disposing && components)
{
components.Dispose
}
That goes for all your comparisons with null -- all are redundant and can be cleared up to [code]if (object)[/code] and [code]if (!object)[/code].
Single-char variables ('e') are a bad habit.
-
@dhromed said:
Take this:
if( disposing ){
if( components != null )
{
components.Dispose();
}
}
You are advised to compact that to:
if (disposing && components)
{
components.Dispose
}
That goes for all your comparisons with null -- all are redundant and can be cleared up to [code]if (object)[/code] and [code]if (!object)[/code].
Single-char variables ('e') are a bad habit.
First, that's generated code created by the MS component designer. It's really best to just leave it alone, hidden behind the #region. Second, object references have no implicit conversion to boolean in C#, so the programmer must necessarily use if (object == null) syntax.
-
Thanks for those links, masklinn.
However, I dont exactly see how the builer pattern would be inducive to cleaner code... (Seems to me that XML lends well to recursion.)
Confused,
John "otac0n" Gietzen
-
object references have no implicit conversion to boolean in C#
Already, my opinion of C# has lowered.
-
You're kidding right?
The absence of a real boolean type has been a thorn in the side of C.
Are you saying that
if (disposing && components)is remotely readable?And finally you can write
if (x == 5)without worrying about the mis-typedif (x = 5)and avoid the obtuseif (5 == x)
-
@IngisKahn said:
Are you saying that
if (disposing && components)is remotely readable?
It's the most readable of all, to use mere humans, which is what a
programming language is for. Humans. [code]if (something)[/code]
basically says "[i]if it's there[/i]" which is a fuzzy concept for
which your brain is VERY thankful, and doesn't require the extra
synaptic cycles to parse the [code]== somevalue[/code]
High-level languages are supposed to make things easier for humans so
those humans can focus on more complex concepts. If a language fails to
cater to humans, it fails a high-level language. Then you might as well
go back to Assembly.
-
Silly me, I thought
ifmeant "if it's true" not "if it's there". :pIt's only through C's lack of a boolean type that
ifhas come to mean "if != 0", not to mention in C# null isn't a fancy way to say 0. But I guess this is getting OT... :)
-
Well, I admit that I'm biased by first coming into contact with ECMA,
rather than C or C++. If I'd come to JS later, I might have considered
it too much of a lackadaisical language. For isntance, I wish the ; was
mandatory. It would remove doubt as to whether your specially formatted
code would work or break, and the code would interpret faster, since
the engine doesn't have to guess, and you'd have multi-line strings
with literal linefeeds, instead of \n.
Here's my summarized opinion on 'boolean context':
- converting {null, '', undefined} -> false is a good feature that
makes life easier for us without introducing ambiguity. These values
are essentially 'nothing'.
- converting 0 to false is something I don't like, since I see 0 as a
real numerical value. It's an int, and it's there. Use [code]variable
== 0[/code], just like you use [code]variable == 'certainvalue'[/code].
- converting {null, '', undefined} -> false is a good feature that
-
Ya, I mostly agree, but I think it's best to keep
<FONT color=#0000ff>if</FONT>as simple as possible. Restricting it to booleans only is a thing of beauty IMO.
-
I only use it in the case of null and undefined. Checking against '' I do 'manually'.
But I'd just hate to be forced to do everything manually.
[code]if (something == null)[/code] looks just as stupid as [code]if (something == false)[/code]
-
I think it's a compiler. Am I right?
-
@thematrixeatsyou said:
I think it's a compiler. Am I right?
not exactly, it "compiles" vector images and renders them.
-
Don't catch general exceptions. Use catch (XmlException ex) to catch specific exceptions, but only if you have specific handling logic that does something useful, such as recovering from the error, logging the error or presenting a useful error message to the user. Otherwise, it's better to remove the try/catch block and let the calling code handle the exception. The source of errors is more likely to be in the calling code, such as providing a non-existing file as a parameter, so if you just let the exception flow to that level, you'll spot those bugs much sooner.
Also, check Microsoft's coding conventions in the MSDN help. Instance variables and local variables should start with a lowercase letter.
-
Hi,
you should pay more attention to your try catch clauses; e.g.
public void LoadImage(string Filename)
{
_File = Filename;
pic = null;
string xml = "";
try
{
TextReader reader = new StreamReader( _File );
xml = reader.ReadToEnd();
reader.Close();
}
catch
{
return;
}
image = new CPG( xml );
Render();
}
if reader.ReadToEnd fails then you will leave the reader open. You should implement a finally clause that closes the reader.
Second, it is not a good practice to leave the function in the middle of the code, it makes it hard to follow the flow of the program. Also, in case of an exception, when you exit the function, the program should be in the same state as when you entered the function. This is ecpesially important when you builds libraries or component for other developers to use.
awi
-
Pay attention to the terminology from the caller's perspective. For example, when you create an instance with the (string fileName) constructor, you can then read the file name from the "File" property. In C#, File implies a System.IO.File object reference rather than the name of the file.
As for the builder pattern mentioned above, think about this -- How useful would it be to create an ImageCPG instance using a no-arg constructor. By having no public contructor and using a static method to build an instance for you, this is expressed a little more elegantly. It's probably a good idea to discard the file name after loading the XML. Life is much simpler if that doesn't cause you to scratch your head two years from now (like when you clone an instance and the filename gets copied, then you save over the original).
What's with the resize timer? That's gonna bite you in the ass later. Is the image guaranteed to be fully rendered in 2 seconds? If you want to redraw while resizing, let it start immediately and interrupt and restart the process at each new size. Then people with reallllly fast computers (that would be nearly everybody in three years) can actually enjoy them. People with really slow computers are going to be screwed anyways, this code isn't going to help them at all. If you want to do flicker prevention, use a model like "no size change in last 0.1 seconds" rather than "2 seconds after resizing starts", or even better -- use double buffering. Double buffering will fix all of the UI uglinesses.
As for pure readability, handling of the group type is a little hard to read. First, a switch is made on the name of the node to set the group type, then another switch actually performs the operation. Often, the two seem to be intermingled. For example, for an inverse, the first switch sets "group = CPGGroupType.Inverse". Then the second switch catches a bunch of node names and then does a buch of ifs and if the group is CPGGroupType.Inverse, then it performs the inverse. It shouldn't take two switches and an if to get such a direct consequence. That code should probably be refactored into a section that determines what actions should be performed and a bunch of routines that do them.
My 2 cents, take them or leave them.
