Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I have seen a video tutorial in C# and I followed it to learn C#. However, I am not sure if the author does follow best practice or not. What I asked myself about is why didn't he create a method to print the speed rather than Console.WritLine that in two different methods As follow:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace understandingOO
{
    class Auto
   {
    public string Make;
    public string Model;
    public int Year;
    public string color;
    public int Miles;
    public int Speed;


    public void Accelerate()
    {
        Speed++;
        Console.WriteLine("Current Speed: " + Speed);

    }

    public void Deaccelerate()
    {
        Speed--;
        Console.WriteLine("Current Speed: " + Speed);
    }
 }
}


using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace understandingOO
{
    class Program
  {
    static void Main(string[] args)
    {
        Auto myCar = new Auto();
        myCar.Make = "Toyota";
        myCar.Miles = 5000;
        myCar.Model = "Cutlas..";
        myCar.Speed = 0;
        myCar.Year = 2011;

        myCar.Accelerate();
        myCar.Accelerate();
        myCar.Accelerate();

        myCar.Deaccelerate();

        Console.ReadLine();

    }
}
}
share|improve this question
    
Too short; does not matter. Why did he place using outside of a namespace? StyleCop would be angry. Why did he import LINQ & co? –  Job Jul 22 '11 at 3:57
add comment

closed as primarily opinion-based by MichaelT, gnat, GlenH7, Kilian Foth, Martijn Pieters Feb 18 at 8:51

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise.If this question can be reworded to fit the rules in the help center, please edit the question.

6 Answers

up vote 7 down vote accepted

You are right.. He should have made a method PrintCurrentSpeed(). This is a basic tutorial and he probably wanted to keep it simple.

share|improve this answer
    
... and the "Current Speed:" string should probably be a localized string. ;p –  Steven Jeuris Jul 21 '11 at 21:56
1  
...and a method named accelerate or deaccelerate shouldn't be printing anything. Of course, this is just example code and best practices get suspended for clarity. –  JohnFx Jul 21 '11 at 23:13
2  
...plus it's decelerate, not deaccelerate. –  tdammers Jul 21 '11 at 23:42
    
@tdammers - But i dont see a definition for a decelerate method, just the deaccelerate method. –  Chad Jul 22 '11 at 16:19
add comment

If you are worried about "Best Practice", he should have made the variables private and created methods to access them, created a Constructor and Destructor for the class, commented the code, the magic numbers 5000 and 2011 and strings should be defined constants. I am sure others can add to this.

However all that would get in the way of the purpose of the program, (an introductory tutorial). In fact, if he had written this program to "Best Practice" standards, it would no longer fulfill it's purpose, making it defective even though the program outputs are the same.

Often best practice is choosing, for valid reasons, not to do "Best Practice", which is why "Best Practice" is so rarely practiced, and even more rarely best practice.

share|improve this answer
    
... and the "Current Speed:" string should probably be a localized string. ;p –  Steven Jeuris Jul 21 '11 at 21:57
add comment

My guess would be that if the intention was to teach a beginner that principles like DRY may not be the focus. Thus, this isn't perfect as to try to get there may be confusing to people that may have trouble understanding methods.

share|improve this answer
add comment

Multiple things wrong with the given code:

  1. He should've used properties instead of fields for public access.

  2. Speed should be a property with a { get; private set; } signature. Since methods Accelerate and Decelerate are used to control the speed, it makes no sense to be able to change the speed "manually" by editing the field value. A lot of .NET classes work this way in that they prohibit you from messing with internal values directly.

  3. I disagree with one of the above posts which states that various values should've been defined as constants. It makes no sense to do that since it's arbitrary data. What the programmer should've done is creating a constructor for the class. Having a constructor atomizes creation operation and in a way guarantees that the object is now ready to be used. By manually setting the values from other classes, you never know when the class is finally ready to be utilized.

share|improve this answer
    
I agree, but this is tutorial code. The biggest no no is having the object write to the console directly. Other than that , I think it is OK tutorial wise. –  Jon Raynor Jul 22 '11 at 3:13
add comment

I'm just going to guess, that since it is a simple example, he used writing to console as the simplest possible way of showing what's going on when methods are called.

In 'real' applications, you usually don't print/log changes in object variables. You usually check their values only when needed. In this example you woud not have a PrintSpeed() method. Rather, you'd have GetSpeed(), which could be used by any other object, to either print the speed, or use for some calculations.

share|improve this answer
add comment

Send the correction to author.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace understandingOO
{
    class Auto
   {
    public string Make;
    public string Model;
    public int Year;
    public string color;
    public int Miles;
    public int Speed;


    public void Accelerate()
    {
        Speed++;
        //Accelerate shouldn't write to the console
        //Console.WriteLine("Current Speed: " + Speed);

    }

    public void Deaccelerate()
    {
        Speed--;
        //Deaccelerate shouldn't write to the console
        //Console.WriteLine("Current Speed: " + Speed);
    }
 }
}


using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace understandingOO
{
    class Program
  {
    static void Main(string[] args)
    {
        Auto myCar = new Auto();
        myCar.Make = "Toyota";
        myCar.Miles = 5000;
        myCar.Model = "Cutlas..";
        myCar.Speed = 0;
        myCar.Year = 2011;

        myCar.Accelerate();
        myCar.Accelerate();
        myCar.Accelerate();

        myCar.Deaccelerate();

        //Show car speed
        Console.WriteLine("Current Speed: " + myCar.Speed);


        Console.ReadLine();

    }
}
}
share|improve this answer
    
Did you run StyleCop on your new code? –  Job Jul 22 '11 at 3:58
1  
@Job - No, does the SO answer rich text box come with style cop enabled? :) –  Jon Raynor Jul 22 '11 at 4:07
add comment

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