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 have some pretty basic Regex that scans the output of a HTML file (the whole document source) and attempts to extract all of the absolute links that look like images. Whether they are actually images or not isn't too important, as those checks would be made later.

I am just trying to see if this Regex looks okay, since I kind of hacked it together from various sources.

The Raw Regex Pattern:

\bhttps?:[^)''"]+\.(?:jpg|jpeg|gif|png)

The Regex in Practice (I am coding in Railo - an open source CFML engine):

<cfset variables.getImageURLs = reMatch('\bhttps?:[^)''"]+\.(?:jpg|jpeg|gif|png)', variables.getDocument) />

NOTE: variables.getDocument contains the raw HTML code.

It seems to work great right now and I'm not seeing any issues, but just wanting to know if you can see any potential pitfalls with this method? Is there room for improvement?

Many thanks, Mikey.

share|improve this question
    
Someone has to link it, do not parse html with regex: stackoverflow.com/a/1732454/7602 –  konijn Feb 19 '13 at 14:37
    
@tomdemuyt Nice and enjoyable read! However, in my case, it's the best thing going right now and I only need it for the simplest of tasks, nothing complicated other than grabbing a few links. –  Michael Giovanni Pumo Feb 19 '13 at 15:01
add comment

3 Answers

Have you tested your regex with lines such as : http://example.com/ file.png

(note the space before file).

It matches such lines. Is it what you intend ?

As your question is about efficiency, you should consider having only valid char for url so that it rejects string sooner (instead of simply excluding some invalid chars)

share|improve this answer
    
Ahh, good catch! Any idea how I can adapt what I have to avoid this? Thanks. –  Michael Giovanni Pumo Jan 4 '13 at 11:11
    
@MichaelGiovanniPumo I think adding \s into the disallowed characters section should prevent this: \bhttps?:[^)''"\s]+\.(?:jpg|jpeg|gif|png) –  tiffon Jan 4 '13 at 12:22
    
Next catch : http://exemple.com/folder.png/file.txt –  sfk Jan 4 '13 at 15:40
    
@sfk Good catch - any ideas on how to end the match after one of the extensions has been met? That might solve it. My Regex knowledge is still very much in the novice stage! Thanks. –  Michael Giovanni Pumo Jan 7 '13 at 15:00
add comment

If you're truly only looking for images, why not parse any src attributes you can find? As it stands, all of the provided Regular Expressions will fail on local images, as well as on dynamically generated images, such as:

<img src="/local/images/get_profile_pic.php?id=12345" title="John Doe" />

This is perfectly valid, and none of the other expressions would capture them. Generally, for parsing HTML, you're better of traversing it with something like an XML reader, or a DOM builder, depending on how well the HTML is formed.

Ben Nadel has a pretty good blog article on working with badly formed HTML and translating into an XML document. Once you have an XML document, you can unleash the power of XPath to do some very nice searching.

If, however, you just want a quick and dirty RegEx to get the job done, I'd recommend actually using the underlying Java Regular Expression library, as it's probably more efficient when matching multiple items over a large document.

The main RegEx I'll be using is as follows:

<img\s+[^>]*?src=("|')([^"']+)\1

Which looks for the src attribute in img tags. You can vary this as you see fit.

<cfset html = variables.getDocument /> <!--- your HTML --->
<cfset pattern = CreateObject("java","java.util.regex.Pattern").compile('(?i)<img\s+[^>]*?src=("|')([^"']+)\1') />
<cfset matcher = pattern.matcher(html) />

<!--- loop through the matches --->
<cfloop condition="matcher.find()">
    <cfset src = matcher.group(2) />
</cfloop>

UPDATE : Here's an explanation of the pattern I chose:

<img           - Literal string "<img", match the opening tag
\s+            - Match one or more whitespace characters, so <img\t is valid
[^>]*?         - Lazily match any character that is not a '>' while looking for the next literal string
src=           - Literal string "src="
("|')          - Match either a single or a double quote, both are valid in HTML
([^"']+)       - Match anything that isn't a single or double quote. Note: You *could* use [^\1] here, however this way the match will reject malformed HTML attributes that have mismatched quotes
\1             - Match the value of the first group (either a single or double quote)
share|improve this answer
    
Thanks Jason. As it happens, I already utilise jSoup in my application. So getting image SRC contents is extremely trivial. However, I need to get more than just the contents of SRC attributes in image tags. Sometimes, references to images are made in CSS, or perhaps inline CSS on HTML tags (for things like logos etc). I want to be able to retrieve as many as possible you see. –  Michael Giovanni Pumo Feb 19 '13 at 12:15
    
Then a more general solution can be used, however you're still going experience the same issues as I mention here. It may be easier to write patterns to match each way that an image can be referred to (there is a finite, documented list of ways this can be done) –  Jason Larke Feb 19 '13 at 16:48
    
Yep, I understand. As it happens, the regex part is just to get URL's that look like images. I will do various checks there after to sanitize them, like if it's a valid URI to start with, mime type checks etc. Thanks for your input though - it's an insightful read and will help! –  Michael Giovanni Pumo Feb 19 '13 at 17:05
add comment

@sfk find next catch, link will be: http://exemple.com/folder.png/file.txt I use Negative lookahead. Specifies a group that can not match after your main expression (ie. if it matches, the result is discarded). For testing RegEx i used http://gskinner.com/RegExr/

\bhttps?:[^)''"]+\.(?:jpg|jpeg|gif|png)(?![a-z/])

Links as http://exemple.com/folder.png/file.txt ignored well.

share|improve this answer
    
This didn't seem to work very well, are you sure this is correct? It seemed to match more than it should have done. –  Michael Giovanni Pumo Feb 19 '13 at 17:02
    
yes, i updated comment and put link for online RegEx, where i testing –  Ilya Z Feb 19 '13 at 20:34
    
Try it with a whole set of links including good and bad ones, it seems to match more than it should. It event matched a simple domain name? Thanks. –  Michael Giovanni Pumo Feb 20 '13 at 15:21
    
Surprised to hear it. Basic expression is the absolute same as your, I have added a (?![a-z/]) to the end your pattern. What match more than it should? Can you open this link regexr.com?33rg4 to test and create own example? –  Ilya Z Feb 20 '13 at 16:22
add comment

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.