2
\$\begingroup\$

I have been building a site using php and would like advice to improve the structure of my site and the best way to lay out my code for SEO. I'm also a little concerned about echoing large chunks of HTML and how SEO will work with my structure.

Ok here's my structure...

My site contains many pages with only the body content changing for each page. So I have created template functions in PHP for echoing HTML. I use $_GET['page_id'] in a switch statement to determine what page content to load. .

my pages look something like this....

Header();                            //echo header HTML

 switch ($_GET['page_id'])            //Echo body content
 {
   default: homePage(); break;
   case 1:  servicesPage(); break;
   case 2:  galleryPage(); break;
   case 3:  contactPage(); break; 
 }

 Footer();                           //Echo footer HTML

And my php functions look like this

 function header() {
      echo "<div>
             <h1>Header</h1>
            </div>";
 }

 function footer() {
      echo "<div>
             <h1>Header</h1>
            </div>";
}

 function homePage() 
 {
    echo "<div> 
           <h1> Home </h1>
           <ul> 
            <li> Home1 </li>
            <li> Home1 </li>
           </ul>
           <p>This is home page</p>
          </div>";

 }

I'm really looking for any pointers to improve my code but my main concerns are...

Is it ok to echo large chucks of HTML like that from php? especially with my site growing?

Does Google crawler bot still crawl my site ok using $_GET['page_id'] for all my pages?

P.S I'm new here, so sorry if any code formatting is a little off!

\$\endgroup\$
6
\$\begingroup\$

Looking at your switch statement from an SEO point of view, I'd have to say: it's awful (sorry).
One of the first things to look at, when you're interested in SEO, is mod_rewrite. Search engines don't like urls that say nothing:

http://www.my-site.com/1
http://www.my-site.com/2
http://www.my-site.com/3
http://www.my-site.com/3

Doesn't say anything, whereas

http://www.my-site.com/home
http://www.my-site.com/services
http://www.my-site.com/gallery
http://www.my-site.com/contact

If anything, contains a sort-of meaningful keyword in the url from the off. Not only to crawlers love that, so do people browsing your site, for that matter.
Naturally, you have to keep this up, take the services page for example. I'm assuming that this is the page where you list what you do/offer, then all sub-nav urls should look like:

http://www.my-site.com/services/custom/cms
http://www.my-site.com/services/portfolio

And so on...
As to what Topener suggested: require_once 'pages/'.$_GET['page'].'.php'; is a terrible, terrible idea. Using data from the network, data the client can change to determine what files should be loaded is a huuuuge security hole!

From a CodeReview viewpoint:
A switch is all well and good, but it won't be long untill you find yourself scrolling up and down through your code, because the switch-case has grown so large. You'll have to invent hacky ways of dealing with form validation, and other types of client requests (pagination, ajax, file uploads, ...) in a manageable, safe and maintainable way.
A switch can be useful to shorten a lot of if-elseif-elseif-else branches, but not for something as crucial as to determine what page you have to present the client with. Most modern sites would go with a framework that implements the MVC pattern (Symfony2, ZendFW, Yii, ...)
As AD7Six said in his comment: if all you're after is a simple routing solution, there are micro-fw's, too, that might be a better fit for your problem. Even so, most frameworks are more or less modular, and with a dependency manager like composer, you can use the routing module(s) from any framework.

Your functions are also in violation of the PSR standards as defined by PHP-Fig (look for them on github). Classes (objects) start with an upper-case, functions/methods don't. Try to adhere to what little standards/conventions you have

Lastly, this just looks wrong to me:

switch($foo)
{
    default: homePage(); break;
    case 1: ...
}

The defaut case is the fallback case. To me, logic dictates that the default/fallback procedure is the last resort, and it should therefore be written as the last case label. Maybe that's just me, and my (sometimes over eager) tendency to use case fallthroughs:

switch ($foo)
{
    case 1:
    case 2:
        bothDoTheSame();
        break;
   case 3:
       threeSpecific();
   case 4:
       threeAndFour();
       break;
   default: defaultCase();//no break required
}
\$\endgroup\$
  • \$\begingroup\$ +1. Pointing out that there are many existing micro frameworks which already implement simple routing solutions - rather than full stack frameworks (zend, cakephp, symfony) might be useful to future readers since the question as posed is effectively only about routing logic. \$\endgroup\$ – AD7six Dec 3 '13 at 14:31
  • \$\begingroup\$ Thank you, a great help! am I right in thinking that specific security flaw doesn't affect me as i don't use $_GET['page_id'] in my require_once() function? \$\endgroup\$ – Scott Dec 3 '13 at 20:00
  • \$\begingroup\$ @Scott: Not as such... but that doesn't mean there aren't any security issues with your site. But if your code (a switch-case-default) is all there is to it, then you are safe from tinkered urls \$\endgroup\$ – Elias Van Ootegem Dec 4 '13 at 10:58
  • \$\begingroup\$ It is rather arguable, but I tend to belive that having a single switch for all static routes is a huge win. It depends a lot on site complexity, but when your site is only about several pages I think it is much better to use plain self-documented switch instead of bloated frameworks no one knows about. \$\endgroup\$ – sineemore Jul 24 '18 at 8:44
2
\$\begingroup\$

I would not recommend using functions and echo-ing large chunks of HTML. You could just require the files. There is no need for functions. Also, use names instead of ID's

if (in_array($_GET['page'],array('home','contact','test')){
    require_once 'pages/'.$_GET['page'].'.php';
}

And in the files themself, just add the HTML content, and if needed PHP variables:

      <div> 
       <h1><?php echo $_GET['page']; ?></h1>
       <ul> 
        <li> Home1 </li>
        <li> Home1 </li>
       </ul>
       <p>This is home page</p>
      </div>
\$\endgroup\$
  • 2
    \$\begingroup\$ The require_once is a terrible idea! I explain why in my answer, but please, don't just suggest horribly unsafe things, without any form of warning!!! \$\endgroup\$ – Elias Van Ootegem Dec 3 '13 at 13:23
  • \$\begingroup\$ I have to agree with @EliasVanOotegem here. That specific security hole has killed both one and two servers for me. Even though there is an in_array check there, I would use a different way to check it. \$\endgroup\$ – Simon Forsberg Dec 3 '13 at 13:51
  • 3
    \$\begingroup\$ <?php echo $_GET['page']; ?> is also an xss attack waiting to happen. Using files as templates is itself a good suggestion - but because of the simplistic code example this could easily become vulnerable code - for that reason -1. \$\endgroup\$ – AD7six Dec 3 '13 at 14:27
  • \$\begingroup\$ +1 for the suggestion on structure. Apart from the security flaw this was helpful. \$\endgroup\$ – Scott Dec 3 '13 at 20:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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