HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpModerate

Change desktop background

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
changebackgrounddesktop

Problem

I've created a simple program that downloads an image (different image every day), then sets it as my desktop background.

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.

Although I have significantly improved my program structure, I want to know if there are any poor practices currently in my code, or improvements that can be made.

Link to the original program:

The updated, final version is formatted below.

```
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)
{
String URL = getBackgroundURL();
Image background = downloadBackground(URL + getResolutionExtension());
saveBackground(background);
setBackground(background, PicturePosition.Fill);
}

public static String getBackgroundURL()
{
using(WebClient webClient = new WebClient())
{
Console.WriteLine("Downloading JSON...");
String jsonString = webClient.DownloadString("super secret URL");
dynamic jsonObject = JsonConvert.DeserializeObject(jsonString);
String backgroundURL = "image_url.com" + jsonObject.images[0].urlbase;
Console.WriteLine("Downloaded JSON!\n");
return backgroundURL;
}
}

public static Boolean websiteExists(String URL)
{
try
{
HttpWebRequest request = WebRequest.Create(URL) as HttpWebRequest;
r

Solution

These using directives are not needed and can be safely removed:

using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Linq;
using System.Text;
using System.Threading.Tasks;


Method names should be PascalCase:

  • getBackgroundURL => GetImageUrl



  • websiteExists => WebsiteExists



  • getResolutionExtension => GetResolutionExtension



  • downloadBackground => DownloadImage



  • getBackgroundPath => GetImageFilePath



  • saveBackground => SaveBackground - this would be more precise as SaveImageFile



  • setBackground => SetBackground - this would be more precise as SetDesktopWallpaper



URL as part of a member name, should be Url; as a parameter name, should be url.

I would move the nested types PicturePosition and NativeMethods to another file, named after the type (respectively, PicturePosition.cs and NativeMethods.cs).

In this snippet:

RegistryKey key = Registry.CurrentUser.OpenSubKey(@"Control Panel\Desktop", true);
switch (style)
{
    case PicturePosition.Tile:
        key.SetValue(@"PicturePosition", "0");
        key.SetValue(@"TileWallpaper", "1");
        break;


There's a possibility for a NullReferenceException on every access of the key object, including key.Close() - also, since .net 4.0 the RegistryKey implements IDisposable, which means it should be wrapped with a using block and not closed explicitly:

try
{
    using(var key = Registry.CurrentUser.OpenSubKey(@"Control Panel\Desktop", true))
    {
        switch (style)
        {
            //...
        }
    }
}
catch (NullReferenceException)
{
    Console.WriteLine("Specified registry key was not found.");
}


I'm not a big fan of that switch block, but abstracting a concept here would probably be overkill.

The verbatim specifier on @"PicturePosition" and @"TileWallpaper" isn't needed, as there are no escapes/backslashes in these strings.

I like that your type declarations are consistently explicit, but I would personally prefer them consistently implicit (i.e. using var) - but the keyword here is only "consistently", so that's just my own personal preference here.

This notation makes it hard to, say, add attributes to your enums, or just to maintain them in general:

public enum PicturePosition
{
    Tile, Center, Stretch, Fit, Fill
}


Enum members should be laid out vertically:

public enum PicturePosition
{
    Tile,
    Center,
    Stretch,
    Fit,
    Fill
}


This is a nice missed opportunity for string.Concat:

String potentialURL = "_" + resolution.Width + "x" + resolution.Height + ".jpg";


Overall I like your methods, they're small and relatively specialized, and are generally well named. I'd consider regrouping them into similar-themed classes:

  • WebClient and JsonConvert-related stuff into one.



  • IO-related stuff into another.



This would only leave Main and SetBackground in the Program class, resulting in 3 smaller, specialized classes.

Code Snippets

using System.Collections.Generic;
using System.ComponentModel;
using System.Data;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
RegistryKey key = Registry.CurrentUser.OpenSubKey(@"Control Panel\Desktop", true);
switch (style)
{
    case PicturePosition.Tile:
        key.SetValue(@"PicturePosition", "0");
        key.SetValue(@"TileWallpaper", "1");
        break;
try
{
    using(var key = Registry.CurrentUser.OpenSubKey(@"Control Panel\Desktop", true))
    {
        switch (style)
        {
            //...
        }
    }
}
catch (NullReferenceException)
{
    Console.WriteLine("Specified registry key was not found.");
}
public enum PicturePosition
{
    Tile, Center, Stretch, Fit, Fill
}
public enum PicturePosition
{
    Tile,
    Center,
    Stretch,
    Fit,
    Fill
}

Context

StackExchange Code Review Q#71730, answer score: 17

Revisions (0)

No revisions yet.