5
\$\begingroup\$

This program is eventually going to be a screen recorder, but right now I just want it to capture screen images.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Windows;
using System.Windows.Controls;
using System.Windows.Data;
using System.Windows.Documents;
using System.Windows.Input;
using System.Windows.Media;
using System.Windows.Media.Imaging;
using System.Windows.Navigation;
using System.Windows.Shapes;
using System.Drawing;
using System.Windows.Forms;


namespace ScreenRecorder
{
    /// <summary>
    /// Interaction logic for MainWindow.xaml
    /// </summary>
    public partial class MainWindow : Window
    {
        public static bool recording = false;
        public static string saveAddress = @"C:\Users\UserName\Desktop\something.bmp";

        public MainWindow()
        {
            InitializeComponent();
        }

        private void onClick(object sender, RoutedEventArgs e)
        {
            System.Drawing.Image currentImage; 
            if (recording == false)
            {
                currentImage =  this.CaptureScreen();
                recording = true;
                recorderButton.Content = "Stop Recording!";
                currentImage.Save(saveAddress);
            }
            else
            {
                recording = false;
                recorderButton.Content = "Start Recording!";
            }
        }

        private System.Drawing.Image CaptureScreen()
        {
            System.Drawing.Rectangle screenSize = Screen.PrimaryScreen.Bounds;
            Bitmap target = new Bitmap(screenSize.Width, screenSize.Height);
            using (Graphics g = Graphics.FromImage(target))
            {
                g.CopyFromScreen(0, 0, 0, 0, new System.Drawing.Size(screenSize.Width, screenSize.Height));
            }
            return target;
        }
    }
}
\$\endgroup\$
7
\$\begingroup\$

Considering there is almost no code to review, this will be purely stylistic. Remember that criticism is a sign of love.

/// <summary>
/// Interaction logic for MainWindow.xaml
/// </summary>
  • The comment isn't needed, we all know what a codebehind is for. Comments should explain the why, not the what.

  • You should (almost) never have public fields. If they are actually needed outside of the current object's scope then you should make them properties, otherwise private.

  • Are they consciously static? This will mess up your program if you execute it multiple times simultaneously. A static modifier on a field is something you probably won't need very often and should be avoided unless needed.

  • You're hardcoding the user's path. It would be more userfriendly to let them pick this through a FolderBrowserDialog.

  • C#'s naming conventions dictate that methods are UpperCamelCase, not lowerCamelCase.

  • You've already imported System.Drawing, remove all the explicit namespace naming in your code.

  • currentImage is only used inside the first part of your if block so you should declare it there. Always restrict the visibility of variables to the most-restrictive scope possible. You'll notice you won't even need the temporary variable and might as well use CaptureScreen().Save(saveAddress).

  • if(recording == false) can be less verbosely written as if(!recording).

  • Don't use this unless you need it to distinguish between members of different scopes.

  • Use resource files to store your buttons their text so you can easily re-use messages and add additional languages. It also allows you to centrally manage all messages.

  • CaptureScreen() indicates a long-running operation with perhaps events raised to return information. That's not the case here. I suggest naming it GetScreenCapture().

  • Some people prefer using var to - again - be less verbose.

\$\endgroup\$
  • \$\begingroup\$ I'm not sure I quite follow how CaptureScreen() indicates a long running operation. Can you elaborate on that? \$\endgroup\$ – Nathvi Dec 31 '14 at 4:36
  • 1
    \$\begingroup\$ @Nathvi I think he means that, conventionally, when you want to get an instant value of X, you use a method called getX. CaptureScreen, however, sounds like a method which, for example, may be starting a new thread to start recording screenshots until a call to stop is made. \$\endgroup\$ – lealand Dec 31 '14 at 5:54
1
\$\begingroup\$

Eventhandler

The name onClick() is better suited for a method raising an event than for an eventhandler. Usually if your object has an event, you will have a pattern like

public event EventHandler EventName;
protected virtual void OnEventName(EventArgs e)
{
    EventHandler handler = EventName;
    if (handler != null)
    {
        handler(this, e);
    }
}

Unneeded code

This program is eventually going to be a screen recorder, but right now I just want it to capture screen images.

Then you should remove any code which doesn't belong here.

Right now, after the first screen capturing you need to click the button 2 times to get one screenshot.

Simplification

You don't need to create a new Size object. You can either use the target.Size or screenSize.Size property for calling g.CopyFromScreen(). Additional you better create a Size instead of a Rectangle.

Refactoring

    public MainWindow()
    {
        InitializeComponent();
        recorderButton.Content = "Make and save screenshot";
    }

    private void onClick(object sender, RoutedEventArgs e)
    {
        System.Drawing.Image currentImage = CaptureScreen();
        currentImage.Save(saveAddress);
    }   

    private System.Drawing.Image CaptureScreen()
    {
        Size screenSize = Screen.PrimaryScreen.Bounds.Size;
        Bitmap target = new Bitmap(screenSize.Width, screenSize.Height);
        using (Graphics g = Graphics.FromImage(target))
        {
            g.CopyFromScreen(0, 0, 0, 0, screenSize);
        }
        return target;
    }

this looks much cleaner this way. You shouldn't start to implement features you might need in the future.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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