"How to do a code review?" or "How to tell a co-worker that her HTML sucks?"

Question

I just started working for a small web company that has one designer and one programmer. I'm kind of the person in between with experience in both worlds.

The problem I see right now is that the designer seems to be setting the standards, even though her practices are sometimes wrong (such as invalid html: wrapping block level elements with inline elements, or using tables for layout when there is no apparent need). I know she means well, but I also know that the standards that she is "enforcing" are either outdated or not as effective as they should be. Sometimes, it's hard to communicate with her because she seems unwilling to take ideas from other people.

I think doing peer code reviews might help. At one time, I suggested we might have code reviews, but there's only 3 of us, and the programmer and the designer are not always sharing the same code, so does it really make sense to do code reviews?

What should I do?

How can I influence my co-worker to be more open to suggestions, and/or learn some best practices?

Update: Thanks to everyone for all the advice. I found that "asking for help" set my co-worker in a better mood and was much more pleasant to work with as well as being more open to communication.

Answer 1

Find justification as to why you think your techniques are better. Something like the book Refactoring HTML might be useful.

Answer 2

Just also be sure to explain exactly why it sucks.

Bad: Your HTML sucks.

Better: Your HTML sucks because you use a lot of inline styles on HTML elements

Even Better: Your HTML sucks because you use a lot of inline styles, which makes it much more difficult to make aesthetic changes to the entire site at one time, because one must edit each individual tag instead of a single CSS rule which targets each individual tag. Additionally, page load times might be reduced by moving to a single CSS file, since it could be separately cached by the client.

And, obviously, avoiding incendiary language like "your html sucks" is a good idea as well. I'm just using "your HTML sucks" as a placeholder for criticism of any kind.

Answer 3

Tell her to use the validator ;)

Show her "why table design is bad" homepages etc.

http://phrogz.net/CSS/WhyTablesAreBadForLayout.html ...

Answer 4

Let a machine do the dirty work for you. Pass the website through the w3c validators.

http://validator.w3.org

At a minimum all websites should pass the accessibility standards.

Answer 5

The project I'm working on involves only two programmers (including myself), and together we've developed a review system (based on the considerations from "The best kept secrets of peer code review"), and this works out quite well. I've learned quite some neat programming tricks. Conclusion: code review (if done properly) also works in small teams.

The hard part is to convince your colleague that code review is necessary / useful. One trick here is to run her code through the validator on www.w3.org or make her read the book. Try to always be positive, even if you're telling her that her code sucks (instead, try something like 'hey, I've seen this trick. This might work for us as well' to make her change her code). Make sure you never ever say her code sucks.

Answer 6

Organize informal meetings where you discuss other websites and how they can be improved, like every week or every two weeks.

You don't want a small team to bitch on each others work. So an idea might be to pick a website you like or dislike, and discuss with the rest of the team about what is good, and what isn't. Be sure to pick some websites with some more modern ideas. Your outdated co-worker might pick up an idea or two during the discussion.

If you have a larger team you can easily switch to using automated verification mechanisms. But in your case you have a small team, so it will often look like a personal attack. You want to avoid that feeling because it will ruing the working climate. So, talk about the work of others. This is also useful to analyze what the rest of the world is doing.

Answer 7

You (the team) should agree on a mandadory set of rules first. Using tables for layout is prohibited? Fine. Make that an explicit rule. Without a set of "laws", you will have a hard time to tell her which rules she breaks with her code.

Answer 8

One of the concerns the designers have is:

Why should I do it using CSS if my tables and inline style is shown just well?

I've found showing designers Zen Garden homepage helps a lot to understand tableless HTMLs. Zen Garden can be customized by using just a single CSS and has became a very good showcase of beautiful designs that can be achieved using divs. An extra is you can download every CSS and study it, it has been a very good learning source for me.

Many webdesigners will say WOW after looking at the page ;-)

Answer 9

Indeed, you need to find an answer to the question : 'why does this html sucks?' IMO it's very important to be able to phrase problems for yourself before teaching so else. (in that respect see also http://stackoverflow.com/questions/868301/how-can-i-teach-a-know-it-all-beginner-programmer/868395#868395).

Some clues :

• Is the html crossbrowser ? Should it be? Now, or maybe in the future. Show her which browsers give odd effects and show her why her html is difficult to change in order to work.
• Is she mingling presentation and content? Find resources (and there are many) that point out exactly why that is.
• About mingling : is her site maintainable? Is there a clear seperation of code, style (css) and html?
• Is she using content-related stuff as presentation. Like possibly tables, iframes...?
• And most of all : is she adhering a standard (like XHTML). Explain her why that is important an check if the standard is usefull and not obsolete, and check it against a standard-checker, there are plenty on the web.

Especially the last point could be usefull, since it's kind of objective. Adhering a standard is no miracle solution. but at least her html will be consistent and more readable. And, she will be obliged to think about her coding, which is, IMVHO (im my very humble opinon) the start of everything.

Answer 10

To be productive the team must have agreements about each part of work. The productivity will not raise if you will find a way to force designer to write as you want and she isn't. So the only way to have such agreement is communication in which you should gently show how the work can be done in more effective and interesting (I hope learning something new should always be interesting) way. If this isn't the way for you then you probably should think about leaving that team.

Answer 11

Does she lack some knowledge of CSS? May be you have a technical formation problem. Show her examples of good desing. She may need to read some books (paid by your firm) and you need to be subtle so not to ofend her ego. May be something like:

"Hey XXXX, Look at this designs I have just found. It is so clean and neat. And look, it enables us to keep decoupled content and presentation. Why don't we go in this direction. This way a change in presentation does not need to change the code. Don't you think it will be worth to invest some time learning how this guys have done it."

Answer 12

Sell the client on the fact that accessibility requirements are important for the project. Then point out to her that sticking to her old ways is akin to stealing old people's walking sticks and that that's not nice.

Then show your clients/boss how much easier it is to maintain a nice standards compliant site.

Answer 13

You can never win a typical "your code sucks" discussion, such discussions are highly subjective.

When someone suggests technology from another century, I ask them to solve the problem in hand with the technology they are suggesting so intensively. When done properly (by asking in a humble manner: "I don't know if I understand what you mean, but I hope you can demonstrate it for me"), the other part will hopefully get the point.

Answer 14

1) Tip from Hagakure (http://en.wikipedia.org/wiki/Hagakure): talk about your own sucking code, sth. like "I did this ugly table bla bla here, but I am not satisfied with it. Have you any idea how to improve it?" Nobody feels like beeing critizised.

2) have patience, long lasting bad habbits do not go away quickly.

This is the principal mindset. All other answers apply as well. This should do the trick...

Answer 15

There has to be someone higher-up in the company that thinks the websites do not get updated correctly and/or in a reasonable amount of time. If her 'sucky' practices have not driven anyone to this point, they must not be that bad.

Get some user feedback. Offer a solution to someone else's problem who has more authority than you.

Answer 16

For those who are hard in their ways, talk is cheap. I think the best thing to do is actually demonstrate your point. But don't ambush her with all of the things wrong with her code. Instead, just pick one fundamental detail. If you can show her a better way to do just one thing that completely changes her approach, then she'll begin to open up to your ideas.

So... say that you find a way to shrink the HTML of the home page by 5 KB through better use of CSS. Then you can go on to demonstrate how you did it.

I've been on both ends of this. And the one constant is that proof is usually needed to convince someone to change the way they do things.

Answer 17

In a similar vein to what AmmoQ suggested (I don't have enough karma to comment on their post) - Instead of "laws" have documented "Code Standards." It's a lot of work, but a non-confrontational way to get everyone on board with better code quality. If you have some vendor code that is poor-quality, use that as an excuse for needing documented Code Standards. Otherwise, just use something like "reinforcing our commitment to high-quality code." Once people are receptive, do your research, write the document, and then review it w/ your colleagues. You can include citations to back up the standards, to show they are not subjective, but actual "Best Practices."

Also, you could suggest a little "continuing education" - a lunchtime book club starting with the CSS Zen Garden book.

Your coworker may just be uninformed and she knows that what she does "works." Not wanting to mess around with something that works is pretty understandable. Use psychology to your advantage - sandwich the "your methods are outdated" between "you've got a lot of experience" and "I can learn a lot from you." Etc.

Answer 18

I've always found that going first is a good way to get things started. Send out an email to the group saying, "I find code reviews are a good way of learning and developing best practices in a company and figuring out ways to improve our code. Once a week, let's do a code review where one of us will bring something that they've written and we'll all review it together. I'll go first this week."

That way, you've got the door open and it seems a lot less threatening. Introduce her to your code and your way of doing things that way. Be ready to back up your coding style with documentation ("I used this particular tag instead of a table because it's semantically correct" or "This bit of CSS was tricky, but it saves us using spacer gifs all over the place, which speeds up the site and improves user experience.").

Set up your code reviews with plenty of "good examples" from your code or elsewhere. Don't be afraid to even spend the first couple reviews on popular sites ("let's take a look at what Google does on its search results pages and talk about why they did it" or "this week, we'll look at CSS zen garden and think about how we can apply it to our site."). Warm them up with good examples for a few reviews and they'll be much more likely to get on board with the process and point out faults in their code (and maybe even yours).

Answer 19

I don't think reviews are going to be the venue to discuss this particular issue. What I've seen others do is select another site, or set of sites, which is either the "enemy" or "hero" for your site. Your site either wants to take them down at their own game, or be just like them (the difference is actually minimal depending on how your business competes). Then you track changes that they've done or are doing, and follow that with how their traffic trends seem to be affected (say with Alexa). It should be easy to find that 10/10 of these use a more minimal block-based style. Effectively it becomes your standard to imitate then. It could be though, that your designer is using a set where 10/10 sites have HTML that looks like hers. Now you've got to figure out who is convinced that those are the sites to watch, and how you're going to change their minds.

Did you ever notice how just about every bag manufacturer has a product page that imitates the one on eBags.com? I'm not even sure that eBags is not also imitating the others. Color swatches as tiny boxes smaller than a 32x32 icon, click-to-zoom, rollover a standard view to change the main image. Belkin competes in a different market and doesn't only make bags so doesn't match the group's look. Oh no, Jansport broke with the rollover rule and the zoom rule. Watch this post grow stale as the imitation/research ripples; Northface also uses that zooming style already. (Those links are to a random product page. They illustrate the product page similarity. There is also category page similarity. I'm not actually endorsing any of these items or companies.)

Anyway, my point is, this style and most every other, whether visible or in the HTML or CSS is based on propagation via imitation. Kinda like how bold color-bar T.V. test patterns where popular with German designers' home pages & portfolios 5 years ago, but notably absent from other language groups of designers' home pages. Therefor, change who is being imitated, or (if their HTML looks great) stress that the underlying HTML contains just as many trade-secrets, and point them out.