HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Cinema scheduling

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
cinemaschedulingstackoverflow

Problem

Even though I have asked a question here related to my app using Swing and creating 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 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 varargs feature (see final example above for C... 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() and equals() implementations for Screening appears to be generated by your IDE of choice, but in any case Java 7 already has Objects.hash() and Objects.deepEquals(), so you may want to consider using them instead.



  • I don't have much experience with the new Time APIs of Java 8, but I have a feeling some of your calculations or formatting (see getDurationTime() and setLenghtInHours() in Film) can be easier done without having to perform the calculations yourself.



  • Auditorium.of() seems to be unused, consider removing it?



  • Speaking of your Auditorium class, I find it unusual that its usage seem to necessitate the creation of new instances quite often, such as whenever you have a new Screening (without specifying the auditorium argument). 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.