Skip to main content
Code Review

Return to Answer

Fixed a couple of typos
Source Link
RubberDuck
RubberDuck
  • 31.2k
  • 6
  • 74
  • 177

This is a pet peeve of mine, and an others have disagreed with me about it, so please take it with a grain of salt. I really just can't stand single letter aliases. I mean, is cpu really so long that you can't just use the table name? Do you really need to shorten it to c?

It's lazy and it's the worst kind of lazy. It's the kind of lazy that creates more work. Now, instead of plainly seeing seried_attrseries_attr.attr_name, I have to keep in my mind that saa means "series attribute attribute", and ma means "just plain model attribute" and a is a "model attribute attribute" and... Do you see where this is going? Imagine you're brand new to this code base. Would you be able to easily map the alias names to useful meanings? No. You wouldn't, but you shouldn't have to because alias names should be meaningful.

That said, I do like that you've consistently formatted your code. I especially like that you put some whitespace around your UNION statements. However, I really prefer to see subqueries indented one level. It just makes it easier to follow the logic.

You've also hardcoded C79 into your query. What if I want to find info on D82? I have to change the query in at least 4 places. You should be using a variable for this. Better yet, wrap the query in a stored procedure and use a parameter.

This is a pet peeve of mine, and an others have disagreed with me about it, so please take it with a grain of salt. I really just can't stand single letter aliases. I mean, is cpu really so long that you can't just use the table name? Do you really need to shorten it to c?

It's lazy and it's the worst kind of lazy. It's the kind of lazy that creates more work. Now, instead of plainly seeing seried_attr.attr_name, I have to keep in my mind that saa means "series attribute attribute", and ma means "just plain model attribute" and a is a "model attribute attribute" and... Do you see where this is going? Imagine you're brand new to this code base. Would you be able to easily map the alias names to useful meanings? No. You wouldn't, but you shouldn't have to because alias names should be meaningful.

That said, I do like that you've consistently formatted your code. I especially like that you put some whitespace around your UNION statements. However, I really prefer to see subqueries indented one level. It just makes it easier to follow the logic.

You've also hardcoded C79 into your query. What if I want to find info on D82? I have to change the query in at least 4 places. You should be using a variable for this. Better yet, wrap the query in a stored procedure and use a parameter.

This is a pet peeve of mine, and others have disagreed with me about it, so please take it with a grain of salt. I really just can't stand single letter aliases. I mean, is cpu really so long that you can't just use the table name? Do you really need to shorten it to c?

It's lazy and it's the worst kind of lazy. It's the kind of lazy that creates more work. Now, instead of plainly seeing series_attr.attr_name, I have to keep in my mind that saa means "series attribute attribute", and ma means "just plain model attribute" and a is a "model attribute attribute" and... Do you see where this is going? Imagine you're brand new to this code base. Would you be able to easily map the alias names to useful meanings? No. You wouldn't, but you shouldn't have to because alias names should be meaningful.

That said, I do like that you've consistently formatted your code. I especially like that you put some whitespace around your UNION statements. However, I really prefer to see subqueries indented one level. It just makes it easier to follow the logic.

You've also hardcoded C79 into your query. What if I want to find info on D82? I have to change the query in at least 4 places. You should be using a variable for this. Better yet, wrap the query in a stored procedure and use a parameter.

Source Link
RubberDuck
RubberDuck
  • 31.2k
  • 6
  • 74
  • 177

This is a pet peeve of mine, and an others have disagreed with me about it, so please take it with a grain of salt. I really just can't stand single letter aliases. I mean, is cpu really so long that you can't just use the table name? Do you really need to shorten it to c?

It's lazy and it's the worst kind of lazy. It's the kind of lazy that creates more work. Now, instead of plainly seeing seried_attr.attr_name, I have to keep in my mind that saa means "series attribute attribute", and ma means "just plain model attribute" and a is a "model attribute attribute" and... Do you see where this is going? Imagine you're brand new to this code base. Would you be able to easily map the alias names to useful meanings? No. You wouldn't, but you shouldn't have to because alias names should be meaningful.

That said, I do like that you've consistently formatted your code. I especially like that you put some whitespace around your UNION statements. However, I really prefer to see subqueries indented one level. It just makes it easier to follow the logic.

You've also hardcoded C79 into your query. What if I want to find info on D82? I have to change the query in at least 4 places. You should be using a variable for this. Better yet, wrap the query in a stored procedure and use a parameter.