patternjavaMinor
Cinema scheduling
Viewed 0 times
cinemaschedulingstackoverflow
Problem
Even though I have asked a question here related to my app using Swing and creating
I have this class screening which contains
Screening.java
```
package cinema.schedule;
import cinema.Auditorium;
import cinema.film.Film;
import java.time.LocalDateTime;
import java.time.LocalTime;
public class Screening {
public final Auditorium auditorium;
public final Film film;
private float ticketPrice;
private LocalDateTime startDateTime;
public Screening(Film film, LocalDateTime startDateTime) {
this(new Auditorium(), 0, startDateTime, film);
}
public Screening(Film film, float ticketPrice, LocalDateTime startDateTime) {
this(new Auditorium(), ticketPrice, startDateTime, film);
}
public Screening(Auditorium auditorium, float ticketPrice, LocalDateTime startDateTime, Film film) {
this.auditorium = auditorium;
this.ticketPrice = ticketPrice;
this.startDateTime = startDateTime;
this.film = film;
}
public float getTicketPrice() {
return ticketPrice;
}
public void setTicketPrice(float ticketPrice) {
this.ticketPrice = ticketPrice;
}
public LocalTime getStartTime() {
return startDateTime.toLocalTime();
}
public void setStartTime(LocalDateTime startDateTime) {
this.startDateTime = startDateTime;
}
@Override
public String toString() {
return "Screening{" +
"switcher=" + auditorium +
", film=" + film +
", ticketPrice=" + ticketPrice +
", startDateTime=" + startDateTime +
'}';
}
@Override
public boolean equals(Object o) {
if(this == o) return true;
if(o == null || getClass() != o.getClass()) return false;
Screening screening =
JPanel, I still have other ones about best practice making applications. I'm using Java 8.I have this class screening which contains
Film and Auditorium classes. At the moment the class looks like this:Screening.java
```
package cinema.schedule;
import cinema.Auditorium;
import cinema.film.Film;
import java.time.LocalDateTime;
import java.time.LocalTime;
public class Screening {
public final Auditorium auditorium;
public final Film film;
private float ticketPrice;
private LocalDateTime startDateTime;
public Screening(Film film, LocalDateTime startDateTime) {
this(new Auditorium(), 0, startDateTime, film);
}
public Screening(Film film, float ticketPrice, LocalDateTime startDateTime) {
this(new Auditorium(), ticketPrice, startDateTime, film);
}
public Screening(Auditorium auditorium, float ticketPrice, LocalDateTime startDateTime, Film film) {
this.auditorium = auditorium;
this.ticketPrice = ticketPrice;
this.startDateTime = startDateTime;
this.film = film;
}
public float getTicketPrice() {
return ticketPrice;
}
public void setTicketPrice(float ticketPrice) {
this.ticketPrice = ticketPrice;
}
public LocalTime getStartTime() {
return startDateTime.toLocalTime();
}
public void setStartTime(LocalDateTime startDateTime) {
this.startDateTime = startDateTime;
}
@Override
public String toString() {
return "Screening{" +
"switcher=" + auditorium +
", film=" + film +
", ticketPrice=" + ticketPrice +
", startDateTime=" + startDateTime +
'}';
}
@Override
public boolean equals(Object o) {
if(this == o) return true;
if(o == null || getClass() != o.getClass()) return false;
Screening screening =
Solution
I'll leave it to others to for the GUI review, I only have a suggestion for your
Constructor arguments positioning
It is sometimes recommended to order constructor arguments by their requirements, e.g.
The benefits of doing so are:
-
It helps to prevent careless mistakes when you have the same or convertible argument types. E.g. if you happen to be taking a bunch of
If you accidentally remove the second argument of such a constructor call:
Other possible areas for improvements
Screening class:Constructor arguments positioning
It is sometimes recommended to order constructor arguments by their requirements, e.g.
public Class(A argA) {
this(argA, defaultArgB);
}
public Class(A argA, B argB) {
this(argA, argB, defaultArgsC);
}
public Class(A argA, B argB, C... argsC) {
...
}The benefits of doing so are:
- It is easier to remember what arguments must be supplied, and what has default values.
- It is easier to 'swap' constructors, e.g. during testing, by simply adding or removing the last argument.
- If any arguments are expected to be in an array form, you can make use of the
varargsfeature (see final example above forC... argsC) to slightly simplify the declaration.
-
It helps to prevent careless mistakes when you have the same or convertible argument types. E.g. if you happen to be taking a bunch of
Strings. Assume type B extends A and if the second example above was written as such:public Class(B argB, A argA) {
this(argA, argB, defaultArgsC);
}If you accidentally remove the second argument of such a constructor call:
new Class(objB, objA) to just new Class(objB), objB gets up-casted and is treated like any instance of A instead. This may make understanding the code at 3 am harder... of course, if there's no relationship between these two types, you'll get a straightforward compilation error immediately, which is still bad at 3 am...Other possible areas for improvements
- Your
hashCode()andequals()implementations forScreeningappears to be generated by your IDE of choice, but in any case Java 7 already hasObjects.hash()andObjects.deepEquals(), so you may want to consider using them instead.
- I don't have much experience with the new
TimeAPIs of Java 8, but I have a feeling some of your calculations or formatting (seegetDurationTime() and setLenghtInHours()inFilm) can be easier done without having to perform the calculations yourself.
Auditorium.of()seems to be unused, consider removing it?
- Speaking of your
Auditoriumclass, I find it unusual that its usage seem to necessitate the creation of new instances quite often, such as whenever you have a newScreening(without specifying theauditoriumargument). Your may want to review your model too...
Code Snippets
public Class(A argA) {
this(argA, defaultArgB);
}
public Class(A argA, B argB) {
this(argA, argB, defaultArgsC);
}
public Class(A argA, B argB, C... argsC) {
...
}public Class(B argB, A argA) {
this(argA, argB, defaultArgsC);
}Context
StackExchange Code Review Q#86654, answer score: 4
Revisions (0)
No revisions yet.