The perfect teacher

Kubajzz

See this piece of code:

class Person
{
private string name;
private string sname;

public Person(string argName, string argSName)
{
    name = argName;
    sname = argSName;
}

public string getName()
{
    return name;
}

public string getSName()
{
    return sname;
}

public override string ToString()
{
    return name + " " + sname;
}

}

Now consider these few facts: - the code is in C#, not Java - in case you haven't noticed, this code violates some basic good-style C# coding guidelines - the person who wrote this code teaches C# programming at university - this piece of code was given to his students as a do-it-like-this example - he never, ever uses properties (Java-style getXXX() and setXXX() methods seem to be good enough for him...) - he sometimes names his classes and methods in Czech using diactitics (yes, it compiles fine since VS is Unicode-based, but... WTF?) - And more!

Chris Meech

Kubajzz wrote:

- in case you haven't noticed, this code violates some basic good-style C# coding guidelines

I haven't noticed. Can you provide an explanation with an example? Thanks. :)

Chris Meech I am Canadian. [heard in a local bar] In theory there is no difference between theory and practice. In practice there is. [Yogi Berra] posting about Crystal Reports here is like discussing gay marriage on a catholic church’s website.[Nishant Sivakumar]

musefan

It happens. At University we had teachers that tought us, and teachers that we tought

I may or may not be responsible for my own actions

Kubajzz

Sure :) First of all, properties should be used in C# instead of get() and set() methods wherever appropriate. Public member names should start with a capital letter. Prefixes such as "arg..." are evil. The constructor parameters should be simply "name" and "sname". The name "SName" is a great example of an unclear and unneccessary abbreviation that should be avoided. I was too lazy to search for reference links, this is what I remember from the guidelines. Somebody correct me if I'm wrong...

Chris Meech

Thanks. I understand your points. In this day and age, using "SName" instead of say "Surname", whilst seemingly picky is a good point. Sometimes it's all about the attention to details. :)

Chris Meech I am Canadian. [heard in a local bar] In theory there is no difference between theory and practice. In practice there is. [Yogi Berra] posting about Crystal Reports here is like discussing gay marriage on a catholic church’s website.[Nishant Sivakumar]

_Erik_

Congratulations. You have found the main source of the content in hall of shame: shameful teachers.

RobCroll

I like the way they explicitly declare methods as public when the class is implicitly internal. BTW I am a teacher so if you want to pass, hand in your assignments :)

"You get that on the big jobs."

Kubajzz

RobCroll wrote:

I like the way they explicitly declare methods as public when the class is implicitly internal.

I can't really see anything wrong with that :confused:

RobCroll wrote:

if you want to pass, hand in your assignments :)

I have already got an A, but thanks for the offer :D

OriginalGriff

And he should be shot for concatenating his strings... :laugh:

Real men don't use instructions. They are only the manufacturers opinion on how to put the thing together. Manfred R. Bihy: "Looks as if OP is learning resistant."

RobCroll

Kubajzz wrote:

RobCroll wrote:

I like the way they explicitly declare methods as public when the class is
implicitly internal.

I can't really see anything wrong with that :confused:

The methods are public which would suggest they are available to the world but the class is internal and only accessible within the assembly. Explicitly declaring the "Access Modifier" as public is misleading because it can only be private, internal or protected. Congratulations on the A by the way.

"You get that on the big jobs."

RobCroll

After x iterations of .NET it would be nice to have a constant for space String.Space or String.Character.Space. I guess you could encapsulate the statement getFName() but other than that, what is the problem?

"You get that on the big jobs."

GibbleCH

It should be

    return string.Format("{0} {1}", name, sname);

StM0n

Kubajzz wrote:

- this piece of code was given to his students as a do-it-like-this example

Mhm... I have to admit, my coding style is a little bit Java-like... but my padawan and I agree on that... but to say "do-it-like-this" is a little bit... wow... :doh: BTW: like your argumentation on this[]...:thumbsup:

(yes|no|maybe)*

Jeroen De Dauw

How else would students learn to deal with crappy code? When they graduate, the crappy teacher will just be replaced by a crappy boss and crappy colleagues :)

Jeroen De Dauw (blog | Twitter | Identi.ca)

AspDotNetDev

Seems fine to me.

[Managing Your JavaScript Library in ASP.NET]

AspDotNetDev

Seems like a fine pratice to me. If the class is later changed to public, all the properties would then become visible outside the assembly. This may be the intention. There is also the case of when implementing an interface (the members would have to be public, even on an internal class). While neither of those appear to be the case here, I don't see a downside to making the members public.

[Managing Your JavaScript Library in ASP.NET]

Dalek Dave

Oh dear god no! :)

------------------------------------ I will never again mention that I was the poster of the One Millionth Lounge Post, nor that it was complete drivel. Dalek Dave CCC Link[^] Trolls[^]

BobJanova

Got a citation for that? It would seem to me that string.Format is a lot heavier than concatenation, since it has to parse the format string, work out what the arguments are and then substitute them in. Usually the advice is to use StringBuilder but for such a small example I don't see what's wrong with the concatenation, and Format seems worse.

Mel Pama

If the assembly has more than just this internal class then the members should be public if you want the other classes in the same assembly to access them. Internal is assembly in scope, public is class in scope. Internal limits public access only to that assembly for that internally declared class, all other classes can still acess the internal class.

AspDotNetDev

I know. My point was that if you change the class down the road (e.g., by changing it from internal to public) for whatever reason, then it may be advantageous if you had marked all the class members public in the first place (assuming your change of making the class public implies that you also want the members to be public once that change has been made).

[Managing Your JavaScript Library in ASP.NET]