So, I built an online portfolio for myself, and

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
nickvd
DevNet Resident
Posts: 1027
Joined: Thu Mar 10, 2005 5:27 pm
Location: Southern Ontario
Contact:

So, I built an online portfolio for myself, and

Post by nickvd »

I'd like to know what you all think. Please tear it apart, and tell me what sucks, or if it's good, please stroke my ego ;)

http://portfolio.nvdesign.net

Since this forum is more towards php code, I'd like to post the backend to the site (so very small and simple, but since there's an email form i'd like to get it checked over ;))
index.php

Code: Select all

<?php require_once('inc/pagetop.inc.php'); ?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN" "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en">
<head>
   <meta http-equiv="content-type" content="text/html; charset=utf-8" />
   <meta name="description" content="A Gallery of Designs by Web Developer Nick Van Dorsten" />
   <meta name="keywords" content="web design, portfolio, development, design, html, xhtml, standards compliant, css, web application, ajax, web 2.0" />
   <meta name="author" content="Nick Van Dorsten" />
   <link rel="stylesheet" type="text/css" href="css/style.css" media="screen" />
   <script type="text/javascript" src="inc/jquery.js"></script>
   <script type="text/javascript" src="inc/interface.js"></script> 
   <script type="text/javascript">//<![CDATA[
   $(document).ready(function(){ 
      <?php if ($success==true): ?>
         $('#formSuccess').hide().ScrollTo(1500,'easeboth').BlindDown(1000,null,'bounceout');
         setTimeout(function(){$('#formSuccess').Fold(1000,20,null,'bounceout');},4500);
      <?php elseif (isset($errors)): ?>
         $('#formError').hide().ScrollTo(1500,'easeboth').BlindDown(1000,null,'bounceout');
      <?php endif; ?>
   });
   //]]></script>
   <title>Nick Van Dorsten, Web Design Portfolio</title>
</head>
<body>
<div id="bodyWrapper">
   <div id="header">
      <h1>Web Design Portfolio</h1>
      <div>of</div>
      <h2><a href="mailto:nickvd@gmail.com">Nick Van Dorsten</a></h2>
   </div>
   <p>Outlined below, is a list of many of the websites that I have designed and developed for clients around the world.  Some of the sites are still under development, and as a result only a screenshot can be provided.  However, the sites that are complete and live can be visited by using the link that is provided under the thumbnail.</p>
   <p>I pride myself in designing websites which are standards compliant and as cross-browser as possible, while still maintaining ease of maintenence and a pleasing design. I've also been programming in PHP, JavaScript and mySql for over 5 years, having designed and built many custom web applications for clients, including content management/editing systems, easy news posting scripts, a business contact listing directory, and a custom coupon generator.</p>
   <div id="portfolio">
      <ul><?php include_once('inc/portfolio.php'); ?></ul>
      <div class="clr"></div>
   </div>
   <div id="contactInfo">
      <?php if (isset($formResult)) echo $formResult; ?>
      Nick Van Dorsten,<br/>
      Web Application Developer<br/>
      email: <a href="mailto:nickvd@gmail.com">nickvd@gmail.com</a><br/>
      Please Use This Contact Form If You Have Any Questions:
      <form action="index.php" method="post">
         <fieldset id="contactForm">
            <legend>Email Me</legend>
            <div><label for="name">Name:</label><input type="text" name="name" id="name" value="<?php echo $name; ?>"/><br/></div><br/>
            <div><label for="email">Email:</label><input type="text" name="email" id="email" value="<?php echo $email; ?>"/><br/></div><br/>
            <div><label for="message">Message:</label><textarea name="message" id="message" cols="30" rows="5"><?php echo $msg; ?></textarea><br/></div><br/>
            <div><input type="submit" name="submit" id="submit" value="Send Email" /></div><br/>
         </fieldset>
      </form>
   </div>
</div>  
<div id="footer">Copyright, 2006 Nick Van Dorsten &nbsp;•&nbsp; <a href="http://validator.w3.org/check?uri=referer"><img src="images/xhtml11.png" alt="Valid XHTML"/></a> &nbsp;•&nbsp; <a href="http://jigsaw.w3.org/css-validator/"><img src="images/css.gif" alt="Valid CSS"/></a></div>
</body>
</html>
pagetop.inc.php

Code: Select all

<?php
error_reporting(0);
define('TURNED_ON',true); //do we send emails?
define('CELL',true); // do we send them to my cell phone?
$name = $email = $msg = $success = $errors = $formResult = '';

if (!empty($_POST)) {
   require_once('inc/email.php');
   
   if (!empty($_POST['name'])) $name = preg_replace('/[^a-zA-Z\-_\' ]+/','',$_POST['name']);
   else $errors[] = "Name Empty or Invalid...";
   
   if (!empty($_POST['email']) && preg_match('/^[a-z0-9]+[\w\-_\.]*?[a-z0-9]@[a-z0-9]+[a-z0-9\-\.]*' . '\.(?:com|uk|us|info|biz|gov|net|org|edu|ac|au|ca|de|eu|it|ro|ru|th)$/i',$_POST['email']))
      $email = $_POST['email'];
   else $errors[] = "Email Invalid Please Re-Enter...";
   
   if (!empty($_POST['message'])) $msg = strip_tags(trim($_POST['message']));
   else $errors[] = "No Message Supplied, Please Don't Send Blank Messages...";
   
   //no errors in input, try to send the mail
   if (!is_array($errors)) {
      $success = sendMail($name,$email,$msg);
      if (!$success) $errors[] = "We Are Currently Experiencing Network Difficulties, Please Try Again In A Few Minutes...";      
   }
   
   //prep success/error message for user
   if (is_array($errors)) {
      $formResult = "
         <div id='formError'><div>
            <h1>There Were Errors In Your Form Submission</h1>
            <h3>Please See Below...</h3>
            <ul><li>".implode('</li><li>',$errors)."</li></ul>
         </div></div>";
   } elseif ($success === true) {
      $formResult = "<div id='formSuccess'><div><h1>Your Message<br/>Was Sent Successfully...</h1></div></div>";
   }
}
?>
email.php

Code: Select all

<?php
function sendMail($name,$email,$message) {
   if (!TURNED_ON) return true;
   require_once('Swift.php');
   require_once('Swift/Connection/SMTP.php');
   $error = false;
   $swift = new Swift(new Swift_Connection_SMTP('smtp.gmail.com', SWIFT_SECURE_PORT, SWIFT_TLS));
   $swift->authenticate();
   if (!$swift->hasFailed()) {
      $swift->send(
         'nickvd@gmail.com',
         'portfolio@nvdesign.net',
         'From Portfolio',
         "name: {$name}\nemail: {$email}\nmessage: {$message}"
      );
      if ($swift->hasFailed()) $error = true;
      if (!CELL) {$swift->close();return !$error;}
      $swift->send(
         'my_cell_phone_addy',
         'portfolio@nvdesign.net',
         'From Portfolio',
         "name: {$name}\nemail: {$email}"
      );
      $swift->close();
   } else {$error = true;}
   return !$error;
}
?>
I won't bother posting portfolio.php as it simply echo's the list of sites stored in an array...

I know it's simple, it's just always good to have a second (or third, or fourth, or ... N) pair of eyes.

Tnx
User avatar
s.dot
Tranquility In Moderation
Posts: 5001
Joined: Sun Feb 06, 2005 7:18 pm
Location: Indiana

Post by s.dot »

I like it, simple.

I think to make it look more professional, you could go with a smaller font.
Set Search Time - A google chrome extension. When you search only results from the past year (or set time period) are displayed. Helps tremendously when using new technologies to avoid outdated results.
User avatar
Mordred
DevNet Resident
Posts: 1579
Joined: Sun Sep 03, 2006 5:19 am
Location: Sofia, Bulgaria

Post by Mordred »

Might as well check if it's your sleep period before sending to your mobile ;)
User avatar
Maugrim_The_Reaper
DevNet Master
Posts: 2704
Joined: Tue Nov 02, 2004 5:43 am
Location: Ireland

Post by Maugrim_The_Reaper »

A smaller font would be nice. It is odd to see simplicity on a portfolio site - but I'm not the author, and I'm not questioning his design! :)

Grammer in the text needs a little work IMO. The most obvious one is preceding with commas when not required:
", and"
", which"
", or"

In many cases you should drop the comma - it only interrupts the text flow and brings attention to the grammatical errors.

Your page does not validate as XHTML 1.1. I'll note this is not your direct fault - a linked to web service is responsible I think. Looks like the Google Analyser (isn't that called Urchin?).

Some general observations.

- Email should be validated against an RFC822 regex. You can find a few variants online.
- strip_tags() is not 100% effective against malformed tags. Someone on this forum is working on a HTML Purifier you might find useful, or if you want to prevent any HTML, you can escape the message using htmlentities() which is safest.
- Maybe use ctype_print() to ensure no newlines or other odd characters are being used by folk trying to contaminate email headers - not likely but some do try their best on public email forms :).
User avatar
Maugrim_The_Reaper
DevNet Master
Posts: 2704
Joined: Tue Nov 02, 2004 5:43 am
Location: Ireland

Post by Maugrim_The_Reaper »

And I completely failed to notice this was from Sep 26... :roll:
User avatar
Ambush Commander
DevNet Master
Posts: 3698
Joined: Mon Oct 25, 2004 9:29 pm
Location: New Jersey, US

Post by Ambush Commander »

No, it's okay, because no one had commented on it. I felt pretty bad, but I really didn't have anything to say.

Regarding:
Email should be validated against an RFC822 regex. You can find a few variants online.
Can you point out specifically a good one? I've done some digging around and... well, I just don't trust them.
nickvd
DevNet Resident
Posts: 1027
Joined: Thu Mar 10, 2005 5:27 pm
Location: Southern Ontario
Contact:

Post by nickvd »

Thanks for all the comments/suggestions thus far!

I've re-read all the text on the site and re-worded a few areas, removed a bunch of commas that didnt really make sense after i read it again.

As for the email regex, I found the following function somewhere in my google searches

Code: Select all

function isValidEmail($email){
   $qtext = '[^\\x0d\\x22\\x5c\\x80-\\xff]';
   $dtext = '[^\\x0d\\x5b-\\x5d\\x80-\\xff]';
   $atom = '[^\\x00-\\x20\\x22\\x28\\x29\\x2c\\x2e\\x3a-\\x3c\\x3e\\x40\\x5b-\\x5d\\x7f-\\xff]+';
   $quoted_pair = '\\x5c[\\x00-\\x7f]';
   $domain_literal = "\\x5b($dtext|$quoted_pair)*\\x5d";
   $quoted_string = "\\x22($qtext|$quoted_pair)*\\x22";
   $domain_ref = $atom;
   $sub_domain = "($domain_ref|$domain_literal)";
   $word = "($atom|$quoted_string)";
   $domain = "$sub_domain(\\x2e$sub_domain)*";
   $local_part = "$word(\\x2e$word)*";
   $addr_spec = "$local_part\\x40$domain";
   return preg_match("!^$addr_spec$!", $email) ? 1 : 0;
}
The only thing i dont like, is that it allows far too many "iffy" addresses. 'someone@somewhere' should not be a valid public routable address (probably isnt routable over the internet anyway) yet it is...

I'll be using it for now, unless someone else has a better one to use.

re:
Your page does not validate as XHTML 1.1. I'll note this is not your direct fault - a linked to web service is responsible I think. Looks like the Google Analyser (isn't that called Urchin?).
Yeah, it's google analytics causing that... Unfortunatly, there isnt much i can do about it :( Would it be in my best interests to remove the validator links in the footer? Considering that it "wont validate" it would be going against what I've said in the text.

What I wrote so far was done fairly quick so I could show someone my work. As soon as I find time, I'll be re-designing it to be a little more... "corporate"

I'm in the middle of something right now, but hopefully by tomorrow night I can get the new version uploaded (not much changed as noted above)...

Thanks for all the responses (so far)... I was beginning to think i was invisible... ;)
Mordred wrote:Might as well check if it's your sleep period before sending to your mobile ;)
I've heard of this 'sleep' thing you are talking about... I may have to do more research before I implement a feature involving something I'm not all that familiar with ;)
matthijs
DevNet Master
Posts: 3360
Joined: Thu Oct 06, 2005 3:57 pm

Post by matthijs »

And maybe add a max-width to the site? I like the fact that the site is fluid, but for those people surfing with larger resolutions (1280,1600+) it might be too wide.

Or you could give the text some more margin on the side and line it up with the thumbnails below it. Then you'll have a better line-length and a nicer grid.

You could also use another Doctype (html 4 strict), as you are not using xhtml 1.1 anyway. Last, if you want you could use an <ul><li> for the portfolio "list" of thumbnails. A bit more semantic IMHO.

I do like the simplicity of it all. One page, everything in sight.
nickvd
DevNet Resident
Posts: 1027
Joined: Thu Mar 10, 2005 5:27 pm
Location: Southern Ontario
Contact:

Post by nickvd »

matthijs wrote:And maybe add a max-width to the site? I like the fact that the site is fluid, but for those people surfing with larger resolutions (1280,1600+) it might be too wide.

Or you could give the text some more margin on the side and line it up with the thumbnails below it. Then you'll have a better line-length and a nicer grid.

You could also use another Doctype (html 4 strict), as you are not using xhtml 1.1 anyway. Last, if you want you could use an <ul><li> for the portfolio "list" of thumbnails. A bit more semantic IMHO.

I do like the simplicity of it all. One page, everything in sight.
I'll definatly have to see what it looks like in higher than 1280/1024, I neglected to check...

The thumbnails are indeed using an unordered list.. contained within them is a div (or two, i cant see the code) for positioning and styling of the image/text/etc...

I'll look into changing the doctype, other than not being true xhtml would there be any other reason to change?
AngusL
Forum Contributor
Posts: 155
Joined: Fri Aug 20, 2004 4:28 am
Location: Falkirk, Scotland

Post by AngusL »

Maugrim_The_Reaper wrote:Grammer in the text needs a little work IMO. The most obvious one is preceding with commas when not required:
", and"
Just to pick you up on that grammar issue: a comma before the "and" is a matter of personal style. It's even got a name - the "Oxford Comma".
Post Reply