Set desktop background

Question

I created a simple program that downloads an image (different image every day), then sets it as my desktop background. I feel like the lines of code can be cut down significantly while still remaining easy to understand.

It is a console application, and closes immediately after downloading the image and setting it as my desktop background. It runs every morning at 6:00 AM via Task Scheduler.

What improvements would you make?

using Microsoft.Win32;
using Newtonsoft.Json;
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Drawing;
using System.IO;
using System.Linq;
using System.Net;
using System.Runtime.InteropServices;
using System.Text;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace Background
{
    class Program
    {
        static void Main(string[] args)
        {
            using (WebClient webClient = new WebClient())
            {
            Rectangle resolution = Screen.PrimaryScreen.Bounds;
            string json = webClient.DownloadString("super secret website url");
            dynamic results = JsonConvert.DeserializeObject<dynamic>(json);
            string url = "domain.com" + results.images[0].urlbase;
            if (resolution.Width <= 1920 && resolution.Height <= 1200)
            {
                url += String.Format("_{0}x{1}.jpg", resolution.Width, resolution.Height);
            }
            else
            {
                url += "_1920x1200.jpg";
            }               
            DesktopBackground desktopBackground = new DesktopBackground();
            desktopBackground.Set(url, PicturePosition.Fill);
            }
        }
    }
    public enum PicturePosition
    {
        Tile, Center, Stretch, Fit, Fill
    }
    public class DesktopBackground
    {
        public DesktopBackground() { }
        const int SET_DESKTOP_BACKGROUND = 20;
        const int UPDATE_INI_FILE = 1;
        const int SEND_WINDOWS_INI_CHANGE = 2;
        [DllImport("user32.dll", CharSet = CharSet.Auto)]
        static extern int SystemParametersInfo(int uAction, int uParam, string lpvParam, int fuWinIni);
        public void Set(string URL, PicturePosition style)
        {
            HttpWebRequest httpWebRequest = (HttpWebRequest)HttpWebRequest.Create(URL);
            HttpWebResponse httpWebReponse = (HttpWebResponse)httpWebRequest.GetResponse();
            Stream stream = httpWebReponse.GetResponseStream();
            Image backgroundImage = Image.FromStream(stream);
            int backgroundNumber = 0;
            string directory = Environment.GetFolderPath(Environment.SpecialFolder.MyPictures) + "/Backgrounds/";
            if (!Directory.Exists(directory))
            {
                Directory.CreateDirectory(directory);
            }
            string fullBackgroundFilePath = Path.Combine(directory, String.Format("background{0}.bmp", backgroundNumber));
            while (System.IO.File.Exists(fullBackgroundFilePath))
            {
                backgroundNumber++;
                fullBackgroundFilePath = Path.Combine(directory, String.Format("background{0}.bmp", backgroundNumber));
            }
            backgroundImage.Save(fullBackgroundFilePath, System.Drawing.Imaging.ImageFormat.Bmp);
            RegistryKey key = Registry.CurrentUser.OpenSubKey(@"Control Panel\Desktop", true);
            switch (style)
            {
                case PicturePosition.Tile:
                    key.SetValue(@"PicturePosition", "0");
                    key.SetValue(@"TileWallpaper", "1");
                    break;
                case PicturePosition.Center:
                    key.SetValue(@"PicturePosition", "0");
                    key.SetValue(@"TileWallpaper", "0");
                    break;
                case PicturePosition.Stretch:
                    key.SetValue(@"PicturePosition", "2");
                    key.SetValue(@"TileWallpaper", "0");
                    break;
                case PicturePosition.Fit:
                    key.SetValue(@"PicturePosition", "6");
                    key.SetValue(@"TileWallpaper", "0");
                    break;
                case PicturePosition.Fill:
                    key.SetValue(@"PicturePosition", "10");
                    key.SetValue(@"TileWallpaper", "0");
                    break;
            }
            key.Close();
            SystemParametersInfo(SET_DESKTOP_BACKGROUND, 0, fullBackgroundFilePath, UPDATE_INI_FILE | SEND_WINDOWS_INI_CHANGE);
        }
    }
}

For those who want to test it out, replace the lines between the using condition with the following lines:

string url = "http://www.listofimages.com/wallpapers/2013/10/tarutao-marine-national-park-thailand-boats-crystal-clear-nature-landscape-1800x2880.jpg"; 
DesktopBackground desktopBackground = new DesktopBackground(); 
desktopBackground.Set(url, PicturePosition.Fill);

Answer 1

PInvoke

According to CA1060, methods that use platform invocation services such as SystemParametersInfo should be organized into an internal class called NativeMethods.

Your SystemParametersInfo method declaration has a couple of discrepencies. For one, the documentation describes the method as returning a boolean - you return an integer. Additionally, the arguments are not entirely correct and in my opinion, should have more idiomatic names.

Correcting the method signature to return a bool will enable you to evaluate whether the operation was successful or not in an idiomatic way. Additionally, setting the SetLastError property of the DllImport attribute will also enable you to retrieve the error number in the event of failure using the Marshal.GetLastWin32Error method.

I see that you have defined some constants to use in cooperation with the aforementioned method. In my opinion these should not be declared at the class level and should instead be declared closer to the actual usage. Generally, the smaller the gap between the declaration and the usage, the better.

DesktopBackground Class

It appears to me as though the DesktopBackground class should be static. This is for a couple of reasons. In it's current state, an instance of the class will be utterly pointless as all of the methods are eligible to become static - having to create an ineffectual instance using the new operator will make the calling code ever so slightly more verbose than it needs to be. Secondly, the desktop background is logically something that there can only be one of (in this case anyway) - it seems fitting that it be static to me. While we are talking about classes and instances, know that the empty constructor that you define is pointless and can be removed.

Set Method

You could improve both the method name and the parameter names for the Set method. The method name could perhaps be more descriptive but I think it is obvious what it does - take that as you may. The parameters should perhaps be renamed from URL to url and from position to style. I want to ask you - do you even need the position parameter? The argument appears to be hard-coded. If you can safely remove this dead parameter, you can remove the dead switch statement too but more on that later.

A lot of the nastiness in the Set method comes from the use of HttpWebRequest. I can see why you used this class over a nicer abstraction such as WebClient - you want to save the image in memory before resolving the file path and then writing the image to disc. But what if you could write the image straight to disc? As the code only runs for a few seconds every 24 hours, I suggest that you generate the image path and then download the image straight to that path using the WebClient. This will make your code much more terse. There is a small concern that the path will become occupied during the few seconds that the image is downloading but I severely doubt that will ever happen. In any case, you could employ some kind of safety.

Your code to generate a filename is good enough. I suggest an alternative approach in my code sample just for the sake of it. Just know that if the directory was ever to become sizable that your algorithm (and mine) would probably be pretty slow.

The switch statement is not tremendously pleasant but it is perfectly readable and therefore it is fine. I do not have intimate knowledge of your requirement, but like I mentioned previously, it looks to me as though you do not need that switch statement at all as the argument for the Set method is hard-coded. If that is the case then you could replace the entire switch statement with the one relevant case label.

You might consider wrapping the RegistryKey in a using statement as there is a chance of failure if your application does not have sufficient access privileges. This is unlikely though as you are only accessing the user hive. The nesting will be horrible if you need to keep the switch statement, though.

At the moment your code is super monolithic. You may consider breaking the Set method out into a series of cohesive private methods.

There are other miscellaneous points of concern such as the way that you use string and String interchangeably - the same is true for + and Path.Combine too - you should be consistent in my opinion.

Finally, you might want to surround the native method call in a try-catch block as the method could fail with an exception for any number of reasons.

Here is some code to support my answer:

internal sealed class NativeMethods
{
    // I am not at all intimate with this particular method of the Windows API so I did not 
    // tweak the signature. You should research this, though. 
    [DllImport("user32.dll", CharSet = CharSet.Auto)]
    internal static extern int SystemParametersInfo(
        int uAction, 
        int uParam, 
        string lpvParam, 
        int fuWinIni);
}

public static class DesktopBackground
{   
    public static void SetFromUrl(string url)
    {
        string directory = Path.Combine(
            Environment.GetFolderPath(Environment.SpecialFolder.MyPictures), "Backgrounds");

        Directory.CreateDirectory(directory);

        string path;
        for (int backgroundNumber = 0; ; backgroundNumber++)
        {
            path = Path.Combine(directory, string.Concat("background", backgroundNumber, ".bmp"));
            if (File.Exists(path) == false) break;
        }

        using (var client = new WebClient())
        {
            client.DownloadFile(url, path);
        }

        using (var key = Registry.CurrentUser.OpenSubKey(@"Control Panel\Desktop", true))
        {
            key.SetValue("PicturePosition", "10");
            key.SetValue("TileWallpaper", "0");
        }

        // Remember that the constants only serve as self-documentation in this case.
        const int SET_DESKTOP_BACKGROUND = 20;
        const int UPDATE_INI_FILE = 1;
        const int SEND_WINDOWS_INI_CHANGE = 2;
        NativeMethods.SystemParametersInfo(SET_DESKTOP_BACKGROUND, 0, path, UPDATE_INI_FILE | SEND_WINDOWS_INI_CHANGE);
    }
}

This code is clearly idealistic - you may need to keep the switch statement for example, but hopefully I gave you some ideas.

Answer 2

In addition to ByteBlast's answer

I agree to the using of static so all the methods are static.

The Set() method

is just doing too much. It is

1. creating a directory
2. creating/composing a filename
3. downloading a file from an url
4. saving the image
5. accessing the registry
6. last but not least: setting the background image

So a refactoring is in order.

HttpWebRequest httpWebRequest = (HttpWebRequest)HttpWebRequest.Create(URL);
HttpWebResponse httpWebReponse = (HttpWebResponse)httpWebRequest.GetResponse();
Stream stream = httpWebReponse.GetResponseStream();
Image backgroundImage = Image.FromStream(stream);  

Image backgroundImage = GetImage(URL);  

---

private static Image GetImage(String imageUrl)
{
    HttpWebRequest httpWebRequest = (HttpWebRequest)HttpWebRequest.Create(imageUrl);
    HttpWebResponse httpWebReponse = (HttpWebResponse)httpWebRequest.GetResponse();
    Stream stream = httpWebReponse.GetResponseStream();
    return Image.FromStream(stream);
}

string directory = Environment.GetFolderPath(Environment.SpecialFolder.MyPictures) + "/Backgrounds/";
if (!Directory.Exists(directory))
{
     Directory.CreateDirectory(directory);
}

can be extracted like

private static String CreateBackgroundsDirectory()
{
    string directory = Environment.GetFolderPath(Environment.SpecialFolder.MyPictures);
    directory = Path.Combine(directory,"Backgrounds");
    Directory.CreateDirectory(directory);
    return directory;
}  

There is no need to check if the directory does not exists, because the CreateDirectory method does this itself.

string fullBackgroundFilePath = Path.Combine(directory, String.Format("background{0}.bmp", backgroundNumber));
while (System.IO.File.Exists(fullBackgroundFilePath))
{
     backgroundNumber++;
     fullBackgroundFilePath = Path.Combine(directory, String.Format("background{0}.bmp", backgroundNumber));
}  

we will refactor to a separate method

private static String ComposeFileName(String directoryName)
{
    string fullBackgroundFilePath = Path.Combine(directoryName,
            String.Format("background{0}.bmp", backgroundNumber));
    while (System.IO.File.Exists(fullBackgroundFilePath))
    {
        backgroundNumber++;
        fullBackgroundFilePath = Path.Combine(directoryName,
                String.Format("background{0}.bmp", backgroundNumber));
    }
    return fullBackgroundFilePath; 
}

but we can do better and will add an overloaded method

private static String ComposeFileName(String directoryName, String pattern)
{
    string fullBackgroundFilePath = Path.Combine(directoryName,
            String.Format(pattern, backgroundNumber));
    while (System.IO.File.Exists(fullBackgroundFilePath))
    {
        backgroundNumber++;
        fullBackgroundFilePath = Path.Combine(directoryName,
                String.Format(pattern, backgroundNumber));
    }
    return fullBackgroundFilePath; 
}  

and refactor the former method to

private static String ComposeFileName(String directoryName)
{
    const string pattern = "background{0}.bmp";
    return ComposeFileName(directoryName, pattern);
}

Next is the saving of the Image which we refactor into a method

private static String SaveImage(Image image,System.Drawing.Imaging.ImageFormat imageFormat)
{
    String directory = CreateBackgroundsDirectory();
    String fileName = ComposeFileName(directory);
    String completeFileName = Path.Combine(directory,fileName);
    image.Save(completeFileName , imageFormat);
    return completeFileName;
}

The registry access we will refactor to the method UpdateRegistryInformation

private static void UpdateRegistryInformation(PicturePosition position)
{
    Dictionary<String,String> keyValues = GetKeyValuesByPicturePosition(position);
    RegistryKey registryKey = Registry.CurrentUser.OpenSubKey(@"Control Panel\Desktop", true);
    foreach(KeyValuePair<String,String> kvp in keyValues)
    {
        registryKey.SetValue(kvp.Key,kvp.Value); 
    }
    key.Close();
}  

---

private static Dictionary<String,String> GetKeyValuesByPicturePosition(PicturePosition position)
{
    Dictionary<String,String> keyValues = new Dictionary<String,String>();
    String picturePositionValue = "0";
    String tileWallpaperValue = "0";

    switch(position)
    {
        case PicturePosition.Tile:
            tileWallpaperValue = "1";
            break;
        case PicturePosition.Stretch:
            picturePositionValue = "2";
            break;
        case PicturePosition.Fit:
            picturePositionValue = "6";
            break;
        case PicturePosition.Fill:
            picturePositionValue = "10";
            break;
    }
    keyValues.Add("PicturePosition",picturePositionValue); 
    keyValues.Add("TileWallpaper",tileWallpaperValue);  
    return keyValues;
}  

So the Set() method would look like

public static void Set(string URL, PicturePosition style)
{
   Image backgroundImage = GetImage(String URL);
   String completeFileName = SaveImage(backgroundImage, style);
   UpdateRegistryInformation(style);
   SystemParametersInfo(SET_DESKTOP_BACKGROUND, 0, fullBackgroundFilePath,
         UPDATE_INI_FILE | SEND_WINDOWS_INI_CHANGE);
}