patterncsharpMinor
Daily Desktop Background
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
```
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
{
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
Don't fetch the same URL twice
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
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:
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
the method will return
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
String formatting
This maybe a matter of taste,
but instead of this:
I prefer using
to make the final string structure easier to see thanks to the template,
and to get rid of
Update
Actually, as @mjolka pointed out, when constructing a path,
because the right way to do that is using
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.
using, well-indented, well-spaced (almost, see below).Don't fetch the same URL twice
GetBackgroundURLBase and GetBackgroundTitlefetch 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.