geekhack
geekhack Community => Off Topic => Topic started by: keyboardlover on Fri, 05 August 2011, 14:51:12
-
A kid at work found this today (posted online in a tutorial) and we were cracking up about it.
[ATTACH=CONFIG]23375[/ATTACH]
Explain why this code is terrible and you win a keycap of my choosing!
Edit: Congrats to daemonraccoon and hashbaz! PM me your addresses and I'll send you each a keycap of my choosing.
Also, two big THUMBS UP to findecanor who made a lot of great points about software design!
daemonraccoon's answer was pretty near exactly what I was looking for:
Then the fact that it [the save method] is declared inside of its class and therefore local in scope.
hashbaz got 2nd place since he essentially said this in different words:
Thinking about the MVC clue and daemonraccoon's comment ... is it that the Save() method is too specific to be useful to a controller class? (assuming that the controller deals with many object types, including People).
Findecanor was very close but he didn't really explain the "why":
I would also make "save()" a virtual member function with basic functionality in a superclass.
-
Is this C#? I'm guessing the getters and setters are redundant, since these members are public already and are all intended to be read-write.
-
Yes it's C# and the getters and setters are not the issue.
It's not super obvious at first but once you realize it you have to laugh.
-
Beware. He might send you a \KL key.
Fixed the slash for you.
-
Hmmm. From Microsoft's documentation: "Static methods and properties cannot access non-static fields and events in their containing type." The Save() method is therefore useless?
-
Hmmm. From Microsoft's documentation: "Static methods and properties cannot access non-static fields and events in their containing type." The Save() method is therefore useless?
That is correct, but not the issue. There are no events in the example I gave.
It's really much simpler than that.
-
The Save method should just be
public static void Save()
{
//TODO : Add Some Stuff To Save person...
}
-
Nope, it's simpler than that.
(Well not as simple as Ripster's idea).
-
I dunno man. I mean, Save() doesn't need to be static at all. Does C# have "saving" built in?
-
Explain why this code is terrible and you win a keycap of my choosing!
It's C#.
-
Well, if that method is intended to save the current Person instance, the Person parameter is totally useless.
-
It's C#.
Hahaha
Keep trying guys =)
-
I am not very good at C# so there might be something that I missed, but if it was like this in C++, Java, Ruby, Python or any other language that I know, I would never design it like this.
The name "Person" is not what I would choose for a class with this interface. The name is too generic.
Either you make a class whose name, interface and functionality reflect the role that the person has in the system, or you make the "Person" instances be minimal objects and have other objects for roles and/or relations that refer to them.
Second... The address fields are much too restrictive. I would put the fields inside an "Address" class and add fields for "state" and "country".
Third, it would also be good to have some kind of validation, so that you don't "save" empty instances. Put validation of addresses inside the "Address" class.
I would also make "save()" a virtual member function with basic functionality in a superclass.
When dealing with resources outside your program -- and persistent storage is outside your program -- then you must also be able to handle that the operation could fail. At the very least, make "save()" return an error code, but it might be more appropriate in this context to thrown an exception.
Anyway.. This is from an example, so some may be excused -- if they are outside the point of the example. If this code snippet had been at the centre of the example, then it would have been more serious.
-
Why would you have a static method for the object type defined in the class?
-
Findecanor is the closest yet. He mentioned part of the issue.
Here's a hint, if you're familiar with "model" centered design (mvc, mvvm, etc.) If person is your model, think about how you're going to use that model elsewhere in your application. How are you going to handle making changes to it?
Right now I like Findecanors explanation best. I'll give you guys some more time and explain why later.
-
Also, as Findecanor alluded to, it's a design issue. The issue has nothing to do with syntax, framework, etc.
-
The fact that the Save() is static?
-
Nope.
-
My other points are still valid -- not just the ones that you are referring to. ;)
Yes, the class is incomplete. If the data is supposed to live in persistent storage as a (relational or non-relational) table then you also need some way of loading the data entries. Perhaps some lookup function. That is... if the purpose of the "save()" function isn't just to dump the state to be read while debugging the program.
-
Then the fact that it is declared inside of its class and therefore local in scope.
-
Then the fact that it is declared inside of its class and therefore local in scope.
What is "it" - the save method?
-
Is this the entirety of the class definition? If so, where's the constructor?
(Unless there's some sort of implicit thing going on, I only have very basic experience with C#)
-
What is "it" - the save method?
Yes.
-
No, but the lack of constructor is regardless of the issue.
Edit: DaemonRaccoon got it. Congrats!
-
Thinking about the MVC clue and daemonraccoon's comment ... is it that the Save() method is too specific to be useful to a controller class? (assuming that the controller deals with many object types, including People). In other words, Save() should not be part of the Person class, or it should be a member of a base class that is overridden by Person?
-
Yay! I win!
-
Thinking about the MVC clue and daemonraccoon's comment ... is it that the Save() method is too specific to be useful to a controller class? (assuming that the controller deals with many object types, including People). In other words, Save() should not be part of the Person class, or it should be a member of a base class that is overridden by Person?
Yes, this is correct. I'll give you and daemonraccoon both a keycap.
Like seriously, what are you going to do?
if...
Person myPerson = new Person();
myPerson.Save(); ?????
Bad, bad design. Put the Save() in a DataAccess class and fuggedaboutit!
-
Btw, the kid at work who found this rocks. Only 1.5 yrs experience. =)
He's helped me big time on a couple projects.
-
Sweet, what is our prize?
-
Great work guys. Hope you found this fun. Congrats to daemonraccoon and hashbaz! PM me your addresses and I'll send you each a keycap of my choosing.
Also, two big THUMBS UP to findecanor who made a lot of great points about software design!
Now that you guys understand the issue, you have to laugh when you look at this code again :D
Ripster doesn't find it funny, but that's because he can't write code :D
-
@keyboardlover: PM'd with my address.
-
Thanks KL, this has been fun and edumacational.
-
It's funny cuz it's such a subtle issue. You don't notice it at first. :D
-
Can we have a simpler one of these? My C# skills are degrading.
-
Maybe one of the other code ninjas here can host the next one? :D
Edit: Updated the OP with more info about the winner's answers.
-
(Attachment) 23375[/ATTACH]
Explain why this code is terrible and you win a keycap of my choosing!
kps nailed it before me due to different timezone.
Also:
// TODO : Add some stuff to save the person.
Jesus saves - all others pay cash.
-
Wrong and wrong.
Language geeks. Wouldn't be able to recognize a design problem if it crawled out and bit them!
-
Shouldn't you just serialize the person and leave how you wish to save it as a choice fopr the code that uses the class?
Of course, it could be a naming convention thing, perhaps related to transaction-style behaviour:
person a = new Person(); //a has default values
a.setName("blah"); //blah is stored in a pending fashion, but not yet available if you do getName
Person:save(a); // only now does the above change take effect.
-
Sure, but regardless, saving person will take place elsewhere in the business logic and likely use some sort of dataaccess class to do so. The point is that person and save have nothing to do with each other and therefore should be decoupled.
-
It uses 16bit per character strings, that's likely a huge waste of space, since near half of the available resources could be occupied with nothing. Check if unicode is actually needed at input, add flags...
It uses one object for each person. With object overhead, allocation time and GC time to considering, this Person class can not be used en mass.
So it does not scale.