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

Daily Desktop Background

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

Problem

I've created a simple command-line program that downloads the Bing Image of the Day, then sets it as my desktop background.

It is a console application, and closes immediately after setting my desktop background. It runs daily via Task Scheduler.

I've been working on this as a mini project for a while and would like to know if I am employing any poor coding practices or if there are ways to improve efficiency. The project can be found on GitHub. The executable is located at BingBackground/BingBackground/BingBackground/bin/Release/BingBackground.exe

```
static void Main(String[] args)
{
String urlBase = GetBackgroundURLBase();
Image background = DownloadBackground(urlBase + GetResolutionExtension(urlBase));
SaveBackground(background);
SetBackground(background, PicturePosition.Fill);
}
public static String GetBackgroundURLBase()
{
using (WebClient webClient = new WebClient())
{
Console.WriteLine("Downloading JSON...");
String jsonString = webClient.DownloadString("https://www.bing.com/HPImageArchive.aspx?format=js&idx=0&n=1&mkt=en-US");
dynamic jsonObject = JsonConvert.DeserializeObject(jsonString);
return "https://www.bing.com" + jsonObject.images[0].urlbase;
}
}
public static String GetBackgroundTitle()
{
using (WebClient webClient = new WebClient())
{
String jsonString = webClient.DownloadString("https://www.bing.com/HPImageArchive.aspx?format=js&idx=0&n=1&mkt=en-US");
dynamic jsonObject = JsonConvert.DeserializeObject(jsonString);
String copyrightText = jsonObject.images[0].copyright;
return copyrightText.Substring(0, copyrightText.IndexOf(" ("));
}
}
public static Boolean WebsiteExists(String url)
{
try
{
WebRequest request = WebRequest.Create(url);
request.Method = "HEAD";
HttpWebResponse response = (HttpWebResponse)request.GetResponse();
return (response.StatusCode == HttpStatusCode.OK);
}
catch
{

Solution

Great idea, and very nicely written code, a pleasure to read even for a non-C# guy: decomposed to many small methods with single responsibility, using using, well-indented, well-spaced (almost, see below).

Don't fetch the same URL twice

GetBackgroundURLBase and GetBackgroundTitle
fetch the same URL.
This is unnecessary network bandwidth,
duplicated logic, and duplicated magic strings.
It would be better to have a method to get the jsonObject,
and then pass that to a method that extracts the base URL,
and another that extracts the title.

Avoid magic strings

There are many magic strings scattered around in the code that would be better as constants, defined at the top of the class where they are easy to see:

  • https://www.bing.com/HPImageArchive.aspx?format=js&idx=0&n=1&mkt=en-US



  • _1920x1080.jpg



  • /Bing Backgrounds/



  • M-d-yyyy



Avoid hard coding

It would be nice to control some elements with configuration instead of hard coding, for example:
the destination directory,
the date format,
and possibly the image service URL too in case it might change.

Logging

The program is quite chatty,
writing to console many of the things it is doing.
But one important piece of detail is missed in WebsiteExists:
the method will return false some very different scenarios:

  • 404 error, page not found -- the main intended purpose



  • Other non-200 HTTP status code; quietly ignored



  • Exception thrown; quietly ignored



It might be good to distinguish these different cases,
so that the program reports what happens more accurately than just "resolution AxB was not found".

Related to this, when writing error messages,
probably Console.Error.WriteLine is better than Console.WriteLine.

String formatting

This maybe a matter of taste,
but instead of this:

Environment.GetFolderPath(Environment.SpecialFolder.MyPictures) + 
    "/Bing Backgrounds/" + DateTime.Now.Year.ToString();


I prefer using String.Format,
to make the final string structure easier to see thanks to the template,
and to get rid of ToString() calls, like this:

String.Format("{0}/Bing Backgrounds/{1}", 
    Environment.GetFolderPath(Environment.SpecialFolder.MyPictures), DateTime.Now.Year);


Update

Actually, as @mjolka pointed out, when constructing a path,
String.Format is not a good solution either,
because the right way to do that is using Path.Combine,
as you already did that correctly in other places of the code.

Almost well-spaced

The horizontal spacing is spot on, with spaces around operators.
The indentation is also perfect.
The one thing missing is empty lines between methods,
for improved readability by better visual separation.

Code Snippets

Environment.GetFolderPath(Environment.SpecialFolder.MyPictures) + 
    "/Bing Backgrounds/" + DateTime.Now.Year.ToString();
String.Format("{0}/Bing Backgrounds/{1}", 
    Environment.GetFolderPath(Environment.SpecialFolder.MyPictures), DateTime.Now.Year);

Context

StackExchange Code Review Q#92881, answer score: 4

Revisions (0)

No revisions yet.