Seeking Mentorship for Code Reviews and Improvement

(Mimi) #1

Hello! I discovered this board when attending a WordPress meetup. It’s nice to meet all of you. I am a developer with 3 years agency experience developing both custom WP themes and static site builds. The languages I work with on a daily basis are HTML, CSS/SASS, Javascript/jQuery, and PHP-- pretty standard for a WP developer.

I am seeking mentorship or guidance from an experienced developer who is willing to do some quick code reviews and provide advice. This can be both WordPress related or just general programming best-practices. Even just leaving a comment here would be appreciated, but ideally I am looking for someone with whom I can exchange emails or messages.

My goals are to become a fully-rounded developer, primarily focusing on javascript mvc frameworks. I’m entirely self-taught; if I had the money to attend a coding bootcamp I definitely would, but until then I need to leech knowledge from hands-on experience.

I have some personal projects on my GitHub, as well as the theme for my portfolio site.

My workflow is: local development with MAMP, automation with Gulp (compile sass, validate/minify js, optimize images), and deploying code via git.

I love front-end, I would prefer to work with the presentation view of an application or website as I believe that a client or end-user often judges a site/app based off of fancy UI interactions, rather than fully appreciating functionalities it may offer. However I do recognize that this limits me both professionally and personally.

So please, take a moment of your time to review my work and let me know what I can do to improve.

Thank you in advance.

(Ben) #2

Hi Mimi - welcome to wpchat :slight_smile:

I make themes so that’s where my knowledge lies. As such I have had a look through the theme repository for your website. The one thing that stood out to me is that you’re not escaping anything and so your code is not as secure as it could be. You can read some great documentation on securing your code on the vip docs:

(Mimi) #3

oooh… thank you Ben! This is something I did not know about, and now I know… Thank you for taking the time out of your day =)

(Leland Fiegel) #4

Welcome to WP Chat! Overall, everything seems pretty solid. Like Ben, I am also a theme developer so I’ll just comment on your theme repository.

As a sidenote to what Ben said, it’s worth noting that translations have actually been unescaped in the default themes, with some interesting discussion on Trac as to why. Note that this is just about escaping translations because it can be argued that they can be “inherently trusted.” Everything else should always be validated, sanitized, and escaped.

Personally, I prefer to be “better safe than sorry” so I always escape them anyway, plus it’s required on where I am a theme author along with Ben. :smiley: Just always good to know both sides of the “debate.”

In modern HTML/CSS, I can’t think of any reason to ever use <br>. If you really need a line break, it’s best to wrap it in a <span> or something and style with display: block. That way you have flexibility to remove the line break with a responsive breakpoint. So I’d adjust this line.

In 404.php, I would remove the <br> and then wrap the sentence directly underneath the heading in <p> tags. You can always adjust spacing and such with CSS if necessary.

Looking at functions.php, it’s usually recommended to add theme support via a function that hooks into after_setup_theme. I suppose it’s not that big of a deal though.

Also, that’s an interesting way of combining all the function files. A quick search on GitHub shows that it’s a popular method.

I’d question two things though:

  • Is there a performance issue? I’d imagine referencing specific files would always be faster than searching through an entire directory with a wildcard character. But it’s hard to say for sure without benchmarking it.

  • Is there a security issue? What if a hacker somehow managed to sneak in a file into the /functions/ directory, which was then automatically fired on every page load? I’m just thinking out loud here and if some hacker had access to upload PHP files to your theme directory, this would probably be the least of your concerns anyway.

Speaking of after_setup_theme, instead of hardcoding the <title> tag in header.php, use add_theme_support( ‘title-tag’ ).

It looks like you’re using the Yoast SEO plugin which is probably taking over your title tag in any case, but just a good idea to switch this up and use core functionality when available.

Instead of hardcoding a favicon, you could use WordPress core’s “Site Icon” feature. More info:

In template-contact.php, I noticed <div class=“contact-form-header”>. While this may be the only place this particular stye is used at this time, and I’m nitpicking a little bit here, but you should always keep modularity in mind. Is this a style that could be used elsewhere?

I’d probably use a class like “page-header” so it can be reused in a semantic way in multiple places, if necessary. “Semantic” meaning, it wouldn’t make sense to use “contact-form-header” on a page with no contact form, even though there’s nothing technically stopping you from using that class elsewhere.

Speaking of the contact form page, it looks like you’re running your own contact form functionality through the theme.

It’s totally cool to experiment with this on a personal site, but in any other situation, you can save time by simply adding one of the many polished free contact form plugins, like Ninja Forms, and embed that into a normal page template with a shortcode.

In template-about.php, I’d question the need for a separate page template.

Again, since it’s for your personal site, it doesn’t really matter that much, but think about how the template could be made more flexible.

For example, you could replace the hardcoded links in <div class=“links”> with a widget area. That way, you could technically make unlimited pages with different menus with something like Jetpack Widget Visibility.

Similar to the above point, I’m thinking more in a “public release” rather than a “personal site” mindset, so I wouldn’t necessarily act on this feedback. Just something to think about:

In partials/analytics.php where your GA code is included…

I know that I used to include my GA code at the theme level, and then wondered why my traffic flatlined after I switched themes…not remembering that I included my tracking code in the theme itself.

For a personal site, it’s fine just as long as you’re not as forgetful as me. :slight_smile:

Personally, I’d get in the habit of including the code outside of the theme. Even a mu-plugin that hooked into wp_head would be preferable to mistakenly forgetting to move that code over to the new theme.

As a general note, familiarize yourself with the WordPress Coding Standards, particularly the PHP standards. I noticed some spaces and such were off.

These are things that don’t really affect the execution of the code itself, just something to keep in mind when working with other developers so the codebase can maintain a consistent style.

For example, get_field('company') should be get_field( 'company' ) …notice the extra spaces in between the parentheses.

As someone who works with ACF from time to time, I can tell you their documentation doesn’t always follow WordPress coding standards when it comes to style.

Speaking of mu-plugins, it might be a better idea to register your custom post types and taxonomies through that, rather than at the theme level.

That way, you wouldn’t have to worry about copying over the code to any new theme you switched to.

For a personal site, it’s more of an issue of personal preference. Just another to keep in mind.

Finally, always remember to capitalize the “P” in WordPress. Since you’re hardcoding the content here, for example, capital_P_dangit wouldn’t take effect. :slight_smile:

(Kakoma) #5

Very awesome analysis. Thanks a lot. Been a theme developer for a while but picked up some very useful bits there as well.

Taking after you, to bring the other side of “the debate”, for a personal site, I’d generally steer clear of widgets unless they are doing some stuff that’d take you ages to get right yourself. They do bring the flexibility - yes- but there’s a performance penalty since they involve, among other things, DB calls. I’ll assume that since he’s building the theme, it’s relatively easy to make code changes to wherever widgets would ordinarily be put.

I might even go out on a limb and recommend a custom contact form over a plugin - it’s 4 text fields, a reCAPTCHA field, jQuery validate for the client-side checks and then get the inputs and send them to yourself via email. The plugin’s built to fit setups so there’s definitely overhead in there that you don’t need.

Being a personal site, the gain realized from these compromises may really not be significant but they are good considerations to carry into your next project.

(Nate Wright) #6

In addition to all the great comments here, I’d recommend you spend some time reading the code for the Underscores theme. You’ll pick up lots of good ideas about how to structure code in ways that make it more easy to read and maintain over the long-term.

(Brad) #7

These two plugins have told me all sorts of things and revealed a great deal of info about the themes (and plugins) that we ~you, me, clients,~ tend to use.

Once I started digging into the info that these plugins were showing me, it was both educational, surprising, and eye opening as well.

@spacecow, I hope that helps you and any one else as well. :thumbsup:

(Mimi) #8

Thank you everyone for the response, it is really appreciated. I’m sorry for the late reply, I had a very busy week!