Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I am writing class CollectingClass which collects much data in private members and prints them out later. To encapsulate all the private members, I decided to create a struct Data, keeping all the members (which also enables me to return the struct to the calling class, which is very useful).

I decided that it would be great to move all the print commands to the struct as well (since they don't belong to CollectingClass anymore, according to my feeling of code style)

The printing code is about 150 lines of code. Moving this in the struct seems to be horrible code smell to me (as the header gets huge).

Here are the options and I don't know, which one is the best

  1. Keep the printing code in CollectingClass
  2. Move the printing code as well as the members of the struct to a new class DataClass. Make all members public to allow fast access by CollectingClass
  3. As #2 but create getters and setters for the members, which will probably slow down the execution

What do you think will be the best design? Do you have any other idea?

share|improve this question
Misconception: get/setters will not slow down execution. But it is a bad design decision as it breaks encapsulation (but then from your description the problem is not about encapsulation). – Loki Astari Nov 21 '12 at 15:51
You talk about moving code between files "*.cpp". This is not really useful as you can put methods in any files. What we want to understand is which classes the methods go into and which class you are moving them from. – Loki Astari Nov 21 '12 at 15:52
1  
Also you should post the code. Without it this question is off topic. – Loki Astari Nov 21 '12 at 15:53
IMHO code won't help much, as this is more of a architectural question rather than one that depends on seeing code. – Till Nov 21 '12 at 16:10
1  
Given your inaccurate description of things the code would definitely be useful. English is an in-precise language. On the other hand code is very precise. – Loki Astari Nov 21 '12 at 19:32

closed as off topic by Loki Astari, palacsint, Jeff Vanzella, Trevor Pilley, Brian Reichle Nov 21 '12 at 20:58

Questions on Code Review Stack Exchange are expected to relate to code review request within the scope defined in the FAQ. Consider editing the question or leaving comments for improvement if you believe the question can be reworded to fit within the scope. Read more about closed questions here.

1 Answer

up vote 2 down vote accepted

Structs are usually used for keeping some plain data in cohesion. And it seems to be the case here. There can be useful operations on the data, but they do not form something more abstract with data, so they should not be the methods and objects. In my opinion, you need just function like that:

void print(const Data& inData);

It can be global function or static member of CollectingClass, if you feel it belongs to it.

share|improve this answer
Thanks for your comment, would you add the print function to the CollectingClass or maybe create a new class (or just a new file) to keep the CollectingClass clean from the printing code, which has nothing to do with the collection? – Till Nov 21 '12 at 13:04
I think simple function should be good approach. – GeniusIsme Nov 21 '12 at 13:15
Would you write the static function in the CollectingClass.cpp file or a seperate file or something else? – Till Nov 21 '12 at 14:17

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