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.cpg

    I 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:


    1. 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.


    2. 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.


    3. 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:


    4. The NodeToCPGObject method can really stand to be broken into smaller pieces, especially the long switch/case section.


    5. 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...


    1. 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 =)


  • 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

    Design

    Pattern



  • 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-typed if (x = 5) and avoid the obtuse if (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 if meant "if it's true" not "if it's there". :p

    It's only through C's lack of a boolean type that if has 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].


  • 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.


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.