• Post Reply Bookmark Topic Watch Topic
  • New Topic

OOP, looking for advice/critique  RSS feed

 
Philip Freeman
Ranch Hand
Posts: 30
2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I'm not sure if this is allowed, since I do not technically have an error popping up. Yesterday I posted a "thats wrong OOP" and got some constructive advice.
From it I understood that my approach to it was wrong and after a bit of thinking and learning today I came back hoping to receive advice again.

Hope I'm not breaking any rules or something. I'm just honestly enthusiastic since I think that something clicked in my brain and I have a better understanding now.

Line of thought this time:

Company has offices, departments and of course employees that work on projects that are assigned to teams in some departments. (all that UML to one sentence)




Any critique/advice is welcome and appreciated.

Thanks in advance!
 
Stephan van Hulst
Saloon Keeper
Posts: 7720
142
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Problems:
  • Using concrete types instead of interfaces, e.g. ArrayList instead of Collection.
  • Methods don't show parameters.
  • Company.getDepartments() returns String.
  • No way to add departments or offices to a company.
  • Department has a getDepartment() method?
  • Team has a getTeam() method?
  • Employee has a getEmployee() method?
  • Inconsistent naming: changeManager() but setTeamLeader().
  • The properties of Position have nothing to do with a position at a company.
  • Is a building number always an int?
  • Times should be represented with LocalTime, not int.
  • Many private properties that are unrelated to public API.
  • Many properties expressing relationships that are not represented with connections in the diagram.

  • I suggest you design your public members well before you add private ones. Private members are usually just implementation details, and not appropriate for an UML diagram expressing a first design.

    Another improvement I suggest you make: for many-to-one relationships with "entities" (Employee) rather than "values" (Address), use a Map rather than a Set or List. An Address can be uniquely identified using its properties, so you can add it to a Set. An Employee can not, unless you add an id field. Map entities by their IDs.
    • Post Reply Bookmark Topic Watch Topic
    • New Topic
    Boost this thread!