1
\$\begingroup\$

I'm shooting blind when it comes to coding I'm reading and coding a lot but no one ever looks at my code so it wold be good if someone could look over the project at github and see if I have any major design issues or other bad practice issues that I must work on.

The Github project is here: https://github.com/glennwiz/SupportTool

The files I am most interested in are the following:

DrawController.cs

using System.Drawing;
using System.Windows.Forms;

namespace ServiceTool
{
    public class DrawController
    {
        private PictureBox _drawbox;
        private Bitmap _drawArea;

        public void SetupDrawArea(PictureBox drawbox,Bitmap drawArea)
        {
            _drawbox = drawbox;
            _drawArea = drawArea;
            Graphics graphics = Graphics.FromImage(_drawArea);
            var pen = new Pen(Brushes.Black, 12f);
            graphics.Clear(Color.LightGray);
            graphics.DrawRectangle(pen, 0, 0, drawbox.Width, drawbox.Height);
            graphics.Dispose();
        }

        public bool GetDrawData(int numberMonitor, PictureBox pb, out int locationY, out int locationX, out int width, out int height)
        {
            bool ismain = false;
            width = PcDataCollector.GetMonitordesktopResolution(numberMonitor, "x") / 10;
            height = PcDataCollector.GetMonitordesktopResolution(numberMonitor, "y") / 10;
            locationX = PcDataCollector.GetLocation(numberMonitor) / 10 + (pb.Width / 2 - (width / 2));
            locationY = (pb.Height / 2) - height / 2;
            int nuMain = PcDataCollector.GetMainMonitor();
            if (nuMain == numberMonitor)
                ismain = true;

            return ismain;
        }

        public void DrawTextAndMonitors(int numberMonitor)
        {
            var mypen = new Pen(Color.FromArgb(255, 29, 46, 196), 4f);
            Draw(numberMonitor, mypen);
        }

        public void DrawSelectedMonitor(int numberMonitor)
        {
            var mypen = new Pen(Color.FromArgb(255, 193, 26, 26), 2f);
            Draw(numberMonitor, mypen);
        }

        private void Draw(int numberMonitor, Pen mypen)
        {
            int locationY, locationX;
            int width, height;

            var isMain = GetDrawData(numberMonitor, _drawbox, out locationY, out locationX, out width, out height);
            Graphics graphics = Graphics.FromImage(_drawArea);
            DrawMonitors(width, height, locationX, locationY, graphics, mypen, DrawClass.DrawMonitors);

            // only draw text if in Running mode.
            if (ModeController.mode == Mode.Run)
            {
                WriteTextForBoxes(numberMonitor, graphics, locationY, isMain, width, locationX, height);
            }

            graphics.Dispose();
            _drawbox.Invalidate();
        }

        private void WriteTextForBoxes(int numberMonitor, Graphics graphics, int locationY, bool ismain, int width,
                                               int locationX, int height)
        {
            string xWidth = string.Format("{0}", width * 10);
            string yHeigth = string.Format("{0}", height * 10);

            DrawText(locationX, locationY, ismain, graphics, string.Format("Monitor {0}", numberMonitor + 1), DrawClass.DrawMainText);
            DrawText(locationX + width / 2 - (xWidth.Length * 3), locationY + height - 18, false, graphics, xWidth, DrawClass.DrawResText);
            DrawText(locationX, locationY + height / 2 - yHeigth.Length * 3, false, graphics, yHeigth, DrawClass.DrawResVerticalText);
        }

        public void DrawMonitors(int width, int height, int locationX, int locationY, Graphics graphics, Pen mypen, DrawClass.ScreenDrawDelegate screenDrawDelegate)
        {
            screenDrawDelegate(width, height, locationX, locationY, mypen, graphics);
        }

        private void DrawText(int locationX, int locationY, bool isMain, Graphics graphics, string monitorString, DrawClass.TextDrawDelegate textDrawDelegate)
        {
            textDrawDelegate(locationX, locationY, isMain, graphics, monitorString);
        }
    }
}

DrawClass.cs

using System.Drawing;

namespace ServiceTool
{
    static public class DrawClass
    {
        static readonly Font Fontlarge = new Font("Georgia", 14);
        static readonly Font Fontmedium = new Font("Georgia", 10);
        static readonly Font Fontsmall = new Font("Georgia", 8);

        public delegate void TextDrawDelegate(int locationX, int locationY, bool ismain, Graphics g, string monitorString);
        public delegate void ScreenDrawDelegate(int width, int height, int locationX, int locationY, Pen mypen, Graphics g);

        #region TextDrawLogic

        public static void DrawMainText(int locationX, int locationY, bool ismain, Graphics g, string monitorString)
        {
            g.DrawString(monitorString, Fontlarge , Brushes.Black, locationX + 2, locationY + 2);

            if (ismain)
                g.DrawString("Main Monitor", Fontmedium, Brushes.Black, locationX + 2, locationY + 20);
        }

        public static void DrawResText(int locationX, int locationY, bool ismain, Graphics g, string res)
        {
            g.DrawString(res, Fontsmall, Brushes.Black, locationX, locationY);
        }

        public static void DrawResVerticalText(int locationX, int locationY, bool ismain, Graphics g, string res)
        {
            var drawFormat = new StringFormat();
            drawFormat.FormatFlags = StringFormatFlags.DirectionVertical;

            g.DrawString(res, Fontsmall, Brushes.Black, locationX, locationY, drawFormat);
        }
        #endregion

        public static void DrawMonitors(int width, int height, int locationX, int locationY, Pen mypen, Graphics g)
        {
            g.DrawRectangle(mypen, locationX, locationY, width - 6, height);
        }
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ A) Run StyleCop on all your non-designer code. B) Do not re-create objects that are meant to be reused. \$\endgroup\$
    – Leonid
    Feb 25, 2012 at 20:56
  • \$\begingroup\$ @Leonid B) can be true if recreating an object is very time and or other resources consuming and if the object is stateless in general .for example it's not a good idea to maintain a SqlConnection for a whole project.For example Graphics is an object that uses a Unit of work pattern .You create it, do a bunch of stuffs with it and then flush it and then it's disposed to release the resources that it's using. \$\endgroup\$ Feb 25, 2012 at 21:22

2 Answers 2

2
\$\begingroup\$

One thing that comes to my mind by a quick look, is to use using statement around disposable objects (for example Graphics,Pen,...)

I mean :

using(var graphics=Grapihcs.FromImage(...))
{
....
} 

The benefit of this approach over using graphics.Dispose() , is that if something goes wrong and an exception is thrown the latter won't call the Dispose method but the first will.

\$\endgroup\$
1
  • \$\begingroup\$ Good advice in general, but ... will creating a pen really throw an exception? Effective C# Item 16 suggests that you promote these guys into a static variable books.google.com/… \$\endgroup\$
    – Leonid
    Feb 25, 2012 at 20:54
0
\$\begingroup\$

Var

Use the var keyword when defining method-scope variables for which the right hand side of the definition makes the type obvious. This saves you time when you come to refactor, and is neater.

e.g.

string xWidth = string.Format("{0}", width * 10);

should be

var xWidth = string.Format("{0}", width * 10)

Naming

Don't use single-character variable names or abbreviations. These do nothing but make the code more difficult to comprehend. Extra characters are free, so be descriptive.

public static void DrawMainText(int locationX, int locationY, bool ismain, Graphics g, string monitorString)

should be:

public static void DrawMainText(int locationX, int locationY, bool isMain, Graphics graphics, string monitorString)

Braces

Always prefer to use braces for your if statements, it's cleaner, spaces things out better, and it's a StyleCop violation not to. Additionally, prefer to surround any statement involving braces with empty lines, this improves readability. Additionally, try to give your statements some space. Group together related statements, but leave a gap between unrelated ones.

So

bool ismain = false;
width = PcDataCollector.GetMonitordesktopResolution(numberMonitor, "x") / 10;
height = PcDataCollector.GetMonitordesktopResolution(numberMonitor, "y") / 10;
locationX = PcDataCollector.GetLocation(numberMonitor) / 10 + (pb.Width / 2 - (width / 2));
locationY = (pb.Height / 2) - height / 2;
int nuMain = PcDataCollector.GetMainMonitor();
if (nuMain == numberMonitor)
    ismain = true;

return ismain;

becomes

bool ismain = false;

width = PcDataCollector.GetMonitordesktopResolution(numberMonitor, "x") / 10;
height = PcDataCollector.GetMonitordesktopResolution(numberMonitor, "y") / 10;

locationX = PcDataCollector.GetLocation(numberMonitor) / 10 + (pb.Width / 2 - (width / 2));
locationY = (pb.Height / 2) - height / 2;

int nuMain = PcDataCollector.GetMainMonitor();

if (nuMain == numberMonitor)
{
    ismain = true;
}

return ismain;
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.