Set desktop background

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

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 cl

StackExchange Code Review Q#60755, answer score: 7