What is Good or Bad with this code?

Question

1.00/5 (2 votes)

See more:

I posted this question earlier, but I deleted it by accident.

I have been asked to review the code snippet below and comment on what is good or bad about the code. I've been doing home work all day and my brain is fried. Please help

C#

public void DoSomething(int CustomerID)
{
try
{
var x = GetPersonByID(CustomerID);
if (x.Name.Length() > 25)
{
x.Name = x.Name.Substring(0, 25);
}
DoSomethingElse(x);
}
catch (Exception ex)
{
throw new Exception("Customer not found!");
}
Console.WriteLine("done doing something!");
}

Posted 17-Mar-14 14:35pm

Commish13

Updated 17-Mar-14 15:35pm

v6

Add a Solution

Comments

Sergey Alexandrovich Kryukov 18-Mar-14 0:38am

Please don't remove your original (or last) revision. If you wanted to say thank you, add a comment on the answer.

(My reply will be:
You are very welcome.
Good luck, call again.)

—SA

2 solutions

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

Sergey Alexandrovich Kryukov · Accepted Answer · 2014-03-17T14:51:00

Bad: You use immediate constants: 25, "Customer not found", etc. It makes the code unsupportable. What if you need to change 25 to something else? You will have to change it in two or more places. What if you forget to change it in one place? This is just the disaster. For constants, use explicitly declared const, in some cases — resources. This problem is especially bad with exceptions. How would you filter exceptions when you handle them finally, stopping their propagation. The exception handling is designed to do it by exception types. So, you would need a special exception type. It should be derived not from System.Exception, but from System.ApplicationException or more derived type (just the convention, not critically important, but nevertheless). Other problems: some names are bad: CustomerID, as a method parameter, should be capitalized as customerId (Microsoft naming conventions are pretty good). The names as x and ex are too short. It's a good idea to use only correctly spelled English names in all names; it may greatly simplify finding them.

Good: It's good that you are re-throwing exception and not blocking further exception propagation. You really should let them go and handle in very few, some strategically set points somewhere up the stack (I call them "competency points"). Ultimately, they all should be handled on top of stack for each thread, for UI — in a special points of the main event loop of the application. You only should have changed the exception type to some custom type, like CustomerNotFoundException; I already explained it in the "Bad" section.

Good luck,

—SA

_Maxxx_ · Accepted Answer · 2014-03-17T14:50:00

You need to do some thinking - here are some things you may want to think about

Is it easier to read

C#

if (a=b)
{
dostuff;
}

or

C#

if (a=b)
{
    dostuff;
}

What does it mean to DoSomething? Wuld a better name be "TrimCustomerName" or whatever it is meant to be doing?

When you are catching an exception , is it only going to throw an exception if the customer doesn't exist? what if the database is corrupt, or the network down etc.?

it's trying to trim the name to 25 characters - why? Some comments might help.

it's using the number 25 twice - which means of requirements change and it needs to be changed to 30, it needs to be changed in two places - how could you get around that so there is only one place to change it?

var is valid c# = in this case the x variable will be of the type returned by the GetPersonByID method, so it will be a Person, probably.

Speaking of which- x isn't a very meaningful name...

That should be enough to keep the teacher happy?

What is Good or Bad with this code?

2 solutions

Solution 2

Solution 1

Add your solution here

Preview 0