0

Background

I have been programming mainly in Python in the past years and mostly on my own projects. I have developed some of my own small stilistic conventions like leaving 4 numbers of empty lines between classes and 1-2 lines between methods, etc.

Currently I moved to a company with 8 developers developing mainly in Ruby and Rails. There is some general lack of popular best practices in anything related to code; bad code designs, high coupling, no abstractions, old style coding, no central documentation, mixing of low level stuff in views in an MVC framework etc.

What happened

In an effort to make re-usable code, I committed a small 20-line library like below (I changed some things but tried to keep the main points as closely as I could).

Code:

module CarNaming

  def build_car_name_table()
    # .. stuff here ..
  end

  CAR_NAMES = build_car_name_table()



  # ------------------------------- API --------------------------------

  def car_model_to_name(car_model)
    # .. stuff here ..
  end

end

A few days later I got an email from one of the main lead developers CC:ing the other lead developer and the architect. The email read something like below:

Dear x,

Regarding your latest code on file y, I would suggest to make .. (some technical stuff)..

Also, should I need to remind you to run rubocop against the code before commiting?- it's a mess.

Kind regards, z

I tried to see this with an open eye so I went the other day asking for some clarification. He did explain to me the technical changes and why they were needed. I accepted them as humbly as I could and only raised the lack of documentation that could have prevented this (something he didn't comment on).

However, when it came to the coding style he mentioned some things. The main point was that they try to follow the Rubocop (a code static analyzer) conventions.

The automated warnings were something like this:

Warning: extra blank line at line x
Warning: extra blank line at line y
Warning: extra blank line at line z
Warning: parenthesis not necessary when calling a method
..

That's all fine - we agreed to fix this on the spot. What annoyed me was when he was removing the line with the dashes he exclaimed ".. and this is a mess" (for a second time).

Question

I tried not to take anything personally but I can't deny that I found the "mess" comments rather annoying. Mostly since to me, having white-spaces and some headers to me adds readability than the other way around. But how can you argue about this with someone? Especially if you are the new guy and the other person is in a higher rank than you? Is it even worth talking about?

  • 1
    That API line is a signal that whatever you're attempting to section-off probably needs to be in its own class, file or other organizational structure. Ask your senior developer how they demarcate sections like this, or look at their code and see how they are doing it. – Robert Harvey Aug 4 '16 at 19:37
  • 2
    "Mostly since to me, having white-spaces and some headers to me adds readability than the other way around." And to him, it's the opposite. He isn't wrong just as you're also not wrong. But you're in a team, and you'll have to go with whatever the team chose. If you disagree, you can always argue about it (constructively), but in the end, it's mostly superficial stuff. – Vincent Savard Aug 4 '16 at 19:38
  • 1
    And by "superficial," he means "it's not worth fighting over; do it the way your team is doing it." If you're the only one doing it, you're the one that will have to change. – Robert Harvey Aug 4 '16 at 19:39
  • 2
    In person, ask the guy "Can you tell me what you mean by 'a mess'? I know it's important for the team to agree on standards, so what did I do wrong?" – Bryan Oakley Aug 4 '16 at 19:46
  • 3
    Real questions have answers. Rather than explaining why your situation is terrible, or why your boss/coworker makes you unhappy, explain what you want to do to make it better. For more information, click here... – gnat Aug 4 '16 at 20:31
2

Basically, anything you are not used to, you will will perceive as an unreadable mess. This is why having some automated format police utility is a good thing, then at least you will all get used to the same mess and there will soon be no more triggers for fruitless discussions (the code police utility is right, period).

So, albeit needlessly undiplomatic behavior, you should not take it personally. You are that new mouse that smells a little different. Give it a couple of weeks and you will all like the same code stench because you don't know any different (or will at least have a hard time remembering how you used to do it).

Having said this, I still think Allman style is far superior to K&R/Stroustrup-like formatting which I have to use at work. But my co-workers have seen too many lines of poorly formatted code, it messed up their brains beyond repair. So our formatting police utility is an asshole. But it's still better than not having one along with some programmers with different ideas about the matter.

1

You're writing Ruby now, not Python.

Different languages tend to have different style conventions. Ruby is no exception, here. This isn't an issue of personal preference. The team agrees on a style and it's your responsibility as a team member to follow it.

That said, there are community guides for this sort of thing.

Here's one: https://github.com/bbatsov/ruby-style-guide

Comment style is not really a matter of personal opinion, either. Here's a guide: http://www.rubydoc.info/gems/yard/file/docs/GettingStarted.md

Also, BTW, the code you posted isn't valid Ruby.

  • Sorry for that. I wrote the code here without testing it. I can guarantee that the original code works fine. Also the dash banner I use, I use it regardless of language since it makes it easier for me to find things in long files. – Pithikos Aug 5 '16 at 7:22
1

Experience has taught me that every team has their own quirks and most of them are best left as-is. It sounds like this manager has few real problems to chew on so this will be their soapbox; so let them keep it; it's important to them.

The end goal of any coding standard is to ease the reuse of the code. Either a coworker or you in the future will have to revisit and figure out what the code is doing and make it do other stuff.

Your way looks fine; I don't see a mess, but if everyone built stuff their own way it takes time to spin up and down to each persons style. It's like different authors writing a book. I've been in code that was hard to follow for this very reason.

I have found though that in the long run, the issues that were hard to fix were generally caused by people not doing the big things right, such as hard coding values, improperly using resources, or making an overly complex solution. Simple syntax is very rarely a real issue. I would not worry about it unless this is all the architect seems to worry about. There are bigger issues for them to worry about.

TLTR: I wouldn't sweat it unless this is all they worry about.

  • I think you're missing an important point. It's easier to spot that "big stuff" you mentioned when the picky stuff is all buttoned down. – RubberDuck Aug 4 '16 at 23:49
0

This is merely a matter of poor communication. You were told after the fact to perform a static code analysis. Not your fault.

Many places have what I call "Day 1 Developer Documentation". It simply helps the developers get up to speed.

  1. Download source here...
  2. Download Rubocop...
  3. Download our style convention guide here..
  4. Download these utilities...
  5. Login to our Bug Tracking software here....

Before checking in code:

  1. Run unit a full set of unit tests, make sure all tests pass.
  2. Run static code analyzer (Rubocop), fix all warning errors.
  3. Commit

If these don't documents don't exist, help out the next programmer/coder by creating these documents.

Also, style enforcement should be automated. We use Re-sharper (.Net) and put our coding standards/style in the Resharper configuration file(s). Developers are notified in real time when they are deviating for naming/style conventions. There could be something similar for Ruby, or maybe when one does a build locally, Rubocop is run automatically and issues are flagged/raised prior to check in. Maybe you could automate that as well as part of the build process.

The point is improve it for the next person.

  • I would have done this. The main problem is that there is no main wiki server to contribute to, so each person keeps their own notes. I have raised this issue to one of the seniors and his excuse was "maintainability". – Pithikos Aug 5 '16 at 7:25
  • Superior words make flawless excuses. – Adrian Iftode Aug 5 '16 at 13:34

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