0

I'm working on a bit of code where we're creating object models but the models have generic keys. For example

class myContact {
 var key;
 var value;
}

And then in the code instantiating them as follows

myContact = new myContact()
myContact.key = 'address'
myContact.value = '123 my address'

myContact2 = new myContact()
myContact2.key = 'secondAddress'
myContact2.value = '123 my other address'

And then attaching them to a parent object like

myBank = new company();
myBank.addContact(myContact);
myBank.addContact(myContact2);

To me this feels wrong as the model is so loosely defined. However you do know from the class name it's going to be some sort of contact information.

At the same time I can see why this might be useful as if we want to add a different type of contact in future this kind of methodology makes it easy to do so.

Can any one explain to me if this is good practice? Bad practice and the reasons why?

My initial thoughts:

Good practice

  • Easy to extend contact types in future.
  • Easy to loop through contact information for myBank and get the data.

Bad practice

  • If you want to update a specific contact type you have to loop through to find the correct key.
  • I've never written code like this which is why it feel like bad practice even though it's perfectly acceptable.
  • Anemic model, there's no real need to be a class.
  • There's no defined key so we can't delete or add a contact easily without searching through them all again.
  • There's no definition of what a contact is or should be.

Any thoughts would be greatly appreciated.

edited: Adding some more thoughts as I go alone

Alan Hollis
  • 1,292
  • 3
  • 14
  • 29

2 Answers2

0

in my opinion, there is nothing bad with 'custom fields'. it's rather popular in all systems that allow user to define their own fields (e.g jira, phone books, etc). usually there are some basic, predefined fields/keys like name, id, email etc, which are used e.g. for searching or authentication. the only thing i don't like in this code is the anemic model. if you make a class myClass (change that name!) and then give public access to all its fields, then why do you need a class? class is about encapsulation.

piotrek
  • 13,982
  • 13
  • 79
  • 165
  • Good points about encapsulation, and searching. I think that's where the above code is currently falling down for me. I've done some research and I believe this would fall under the http://en.wikipedia.org/wiki/Entity%E2%80%93attribute%E2%80%93value_model ? Would you agree? – Alan Hollis May 29 '12 at 06:23
  • could be. but it's hard to talk about Entity–attribute–value model when you can see only one class and two examples. for sure it's better to have bank object with collection of `Contact` than bank with `Map` so i'm not saying you should delete the class but be aware what is it. if it servers only for transport values then use a structure instead of class (public fields, no artificial getters and setters etc.). hard to give any better advice without knowing plans for the future of your project – piotrek May 29 '12 at 10:34
0

To me this feels wrong as the model is so loosely defined.

"Doctor, it hurts when I do this" - "Then don't do that".

Do you need a loose model to represent it? Then go for it. If not, if you feel it is only a burden, choose a simpler solution.

Apparently, there is no current need for it. What are the chances of this need to arise in the future? How would such a refactoring impact your codebase? Can you even make assumptions on that yet?

It may make sense in this case to apply the YAGNI principle in this case.

Filippo Pensalfini
  • 1,714
  • 12
  • 16