Simple image capture program

Question

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;
 }
 }
}

Answer 1

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.

• Nathvi: I'm not sure I quite follow how CaptureScreen() indicates a long running operation. Can you elaborate on that?
• lealand: @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.

Answer 2

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.