Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've been making a bot for Reddit. It finds reposts (based on image MD5 comparing) and posts a small comment on the thread (that part will be later, right now it downloads images and compares MD5). If the image is original content, it will add it to its list of hashes. The source code is here. Any suggestions as to what I can improve? Edit: the code I am unsure about is below:

public static bool DownloadImage(string url, bool isDirect) // returns if repost
{ // example url: http://imgur.com/UndN77d
    if (url.Contains('?'))
    {
        url = url.Split('?')[0]; // if it has a query at the end like ?1
    }
    string filename = null;
    if (url.Contains("imgur"))
    { // is imgur, so use ImgurAPI class for it
        string imgurID = url.Split('/')[url.Split('/').Length - 1];
        Console.WriteLine("Processing imgur id {0}", imgurID);
        if (!isDirect)
        { // the sole goal for this block is to get Vars.picToCompare
            string details = Imgur.GetImageDetails(imgurID);
            if (details == null)
            {
                return false;
            }
            Console.WriteLine("Got response: {0}", details); // json
            JObject imgur = JObject.Parse(details);
            if (imgur["success"].ToString() == "false")
            { // auth failed
                Console.WriteLine("Imgur processing failed.. Printing JSON contents: ");
                Console.WriteLine(details);
                Console.ReadKey();
                return false;
            }
            else
            {
                Console.WriteLine("Imgur details recieved successfully!");
                filename = imgur["data"]["link"].ToString();
                url = filename;
                Console.WriteLine("Processed direct URL: " + filename);
                filename = filename.Split('/')[filename.Split('/').Length - 1];
                Console.WriteLine("Processed image filename: " + filename);
                Vars.picToCompare = Path.Combine(Vars.picsFolder, filename);
            }
        }
        else
        {
            Vars.picToCompare = Path.Combine(Vars.picsFolder, url.Split('/')[url.Split('/').Length - 1]); // should be filename
        }
    }
    else
    { // for example mysite.com/blah.jpg
        Vars.picToCompare = Path.Combine(Vars.picsFolder, url.Split('/')[url.Split('/').Length - 1]);
        Console.WriteLine("Processing image url {0}", url);
        Console.WriteLine("Processed image {0}", Vars.picToCompare);
    }
    bool isRepost = false;
    using (WebClient client = new WebClient())
    { 
        if (File.Exists(Vars.picToCompare))
            File.Delete(Vars.picToCompare);
        client.DownloadFile(url, Vars.picToCompare);
        Console.WriteLine("Successfully downloaded image {0}", Vars.picToCompare);
    }
    isRepost = Processing.CompareHash(url);
    if (isRepost)
        Console.WriteLine("This is a repost!");
    else
        Console.WriteLine("This is OC. Adding to hashlist..");
    //foreach (string hash in Vars.totalHashes)
    //{
    //    Console.WriteLine("Written hash to file: " + hash);
    //}
    return isRepost;
}
share|improve this question
1  
You are aware that images which have been loaded and saved one time can be different (when looked at at the byte level)? –  Bobby Feb 5 at 8:07
    
Usually they're fine, if I save the hashes file and rerun the same pictures (it redownloads them) it detects that they're reposts. –  Ilan321 Feb 5 at 8:44
2  
No, what I meant is that a jpeg can change (and also it's MD5 sum) if it is loaded and saved one time in an image processor. So don't be surprised if pictures look the same to you (your eye) but the MD5 sums turn out completely different. –  Bobby Feb 5 at 8:47
add comment

1 Answer 1

up vote 3 down vote accepted
public static bool DownloadImage(string url, bool isDirect) // returns if repost

C# supports something called XML Documentation Comments which allow you to document your code directly while writing it in a processor (as in IDE) friendly way.


public static bool DownloadImage(string url, bool isDirect) // returns if repost

Liar! This does not download the image but does checking on the URL, downloads the image, creates a MD5 sum for that image and then returns if it is already in the list of hashes.

You have one function that does everything, that's bad. Ideally you would split the responsibilities between multiple methods in one class, in some kind of such a structure (pseudo code):

class RepostChecker
    byte[] ComputeMD5(byte[] data)
    byte[] GetImage(string url)
    bool IsKnown(byte[] hash)

All of these methods can be static. So you can chain them like this:

IsKnown(ComputeMD5(GetImage(url)))

Or you can write a convenience method:

bool IsRepost(string url)

Which does the chaining for you. Then you define a class which does hold your hashes:

class Hashes
    static string HashToString(byte[] hash)

    void Add(string hash)
    bool Contains(string hash)
    void Load(whateverDatastoreYouUse)
    void Save(whateverDatastoreYouUse)

Internally this should use a HashSet<String>, not a normal List<String> like your implementation.

These changes would allow a more structured approach to the problem.


if (url.Contains('?'))
{
    url = url.Split('?')[0]; // if it has a query at the end like ?1
}
string filename = null;
if (url.Contains("imgur"))

You should use the Uri class if you work with URLs.

Uri uri = new Uri(url);           // http://imgur.com/blabla?whatever
uri.GetLeftPart(UriPartial.Path); // http://imgur.com/blabla
uri.Host;                         // imgur.com

Ideally you would not have one if {} else {} construct to handle this situation, but an interface which implementations and a static factory:

interface ImageHosterInteractor // Notice that the name is not optimal
    byte[] GetImage(string url)

From this interface you can now derive implementations for imgur and/or any other site. The static factory would look like this:

public static class ImageHosterInteractorFactory
{
    public static ImageHosterInteractor Create(Uri uri)
    {
        if (uri.PathAndQuery.EndsWith(".png")) // Or any other for that matter
        {
            // Implementation which simply loads the data from
            // the given Uri.
            return new ImageHosterInteractors.Plain();
        }

        // The uri does not end with an explicit ending,
        // so there's a good chance that we need to follow
        // redirections or perform some other magic.
        // So we use one of the explicit implementations.

        switch(uri.Host)
        {
            case "imgur.com":
                return new ImageHosterInteractors.Imgur();
                break;

            case "whatever.com":
                return new ImageHosterIntactors.Whatever();
                break;              

            default:
                throw new NotImplementedException("There is no implementation assigned to handle: " + uri.Host);

        }
    }
}

That allows your code to simply process the URL, pass it into the factory and get something back that knows how to treat that site and you don't need to worry about it further down.

Uri uri = new Uri(fromwhatever);
ImagerHosterInteractor interactor = ImageHosterInteractorFactory.Create(uri);
byte[] image = interactor.GetImage(uri);

Also adding new sites to handle is as easy as implementing ImageHosterInteractor and adding it to the factory method.


At the moment it looks like you're saving the images you download, is that necessary or is it enough to simply save the hashes?

share|improve this answer
    
I am saving the images, since I'm running the programs and it's quite funny to see the random images I get. Also, this is one complicated piece of work you got there. I only started C# 2-3 months ago! :O –  Ilan321 Feb 5 at 9:05
    
In that case I'd like to suggest that you read a good book, start with something introductory into C# and OOP and I'm sure the books from Jon Skeet will be a very good read, too. I never even looked at one of these, but I think they're good. Even though it's Java, "Effective Java" is a very good read for general OOP principles. –  Bobby Feb 5 at 9:10
    
Did you meant HashSet<string>? There is no such thing as HashList. –  svick Feb 5 at 11:08
    
@svick: Yes, typo, sorry. –  Bobby Feb 5 at 11:37
    
The book costs money, and I'm afraid I can't even afford the humble indie bundle, so no idea how to get it .-. –  Ilan321 Feb 5 at 16:44
show 2 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.