patterncsharpModerate
Change desktop background
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
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
Method names should be
I would move the nested types
In this snippet:
There's a possibility for a
I'm not a big fan of that
The verbatim specifier on
I like that your type declarations are consistently explicit, but I would personally prefer them consistently implicit (i.e. using
This notation makes it hard to, say, add attributes to your enums, or just to maintain them in general:
Enum members should be laid out vertically:
This is a nice missed opportunity for
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:
This would only leave
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 asSaveImageFile
setBackground=>SetBackground- this would be more precise asSetDesktopWallpaper
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:
WebClientandJsonConvert-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.