Radio with channels

Question

Realize the Radio and Channel classes that represent radio and a radio station. The radio class offers an argumentless constructor and the following methods:

• addChannel: stores and returns a new station, characterized by name, frequency. Attempting to store a station that has the same frequency as an already stored station should result in an exception.
• nearest: accepts a frequency and returns the station with the closest frequency to that given.

Furthermore, if you iterate on a Radio object you get the sequence of stations entered, in increasing order of frequency.

Make sure that the only way to create Channel objects is through the addChannl method.

Observe the following example of use:

Radio r = new Radio();
Channel c1 = r.addChannel("Rai Radio Uno", 89.3);
Channel c2 = r.addChannel("Radio Monte Carlo", 96.4);
Channel c3 = r.addChannel("Radio Kiss Kiss", 101.4);
        
for(Channel c: r) {
     System.out.println(c);
   }

System.out.println(r.nearest(98.1));

Output: Radio Monte Carlo (96.4)

I would like your opinion on my implementation. What can be improved or fixed?

My implementation:

import java.util.*;

public class Radio implements Iterable<Radio.Channel>{

    public static final Comparator<Channel> COMP = (Channel c1, Channel c2) -> {
        return Double.compare(c1.frequenza, c2.frequenza);
    };

    private Set<Channel> channels;
    private List<Channel> my_nesteds;

public Radio() {
    channels = new TreeSet<>(COMP);
    my_nesteds = new ArrayList<>();
}


public Channel addChannel(String nome, double frequenza) {
    Channel new_channel = new Channel(nome,frequenza);
    if(!channels.add(new_channel)){
        throw new RuntimeException("...");
    }
    return new_channel;

}

public Channel nearest(double frequenza){
    double distance = 0.0;
    double min = 0.0;
    boolean first = true;
    Channel ret_channel = null;

    for(Channel cc: channels) {
        if(first) {
            min = Math.abs(cc.frequenza - frequenza);
            ret_channel = cc;
            first = false;

        }else {
            distance = Math.abs(cc.frequenza - frequenza);
            if(min > distance) {
                min = distance;
                ret_channel = cc;
            }
        }
    }
    return ret_channel;
}


@Override
public Iterator<Radio.Channel> iterator() {

    return new Iterator<>() {
        int index = 0;
            @Override
            public boolean hasNext() {
                if(index < channels.size()) {
                    return true;
                }
                return false;
            }

            @Override
            public Channel next() {
                if(index < channels.size()) {
                    Channel c = my_nesteds.get(index);
                    index++;
                    return c;
                }
                throw new NoSuchElementException();
            }
    };
}


/* **************************************************** */
protected class Channel{

    private String nome;
    private double frequenza;

    public Channel(String nome, double frequenza) {
        this.nome = nome;
        this.frequenza = frequenza;
        my_nesteds.add(this);
    }

    @Override
    public String toString() {
        return nome+" "+"("+frequenza+")";
    }


    @Override
    public boolean equals(Object o){
        if(!(o instanceof Channel)){ return false; }
        Channel c = (Channel) o;
        if(COMP.compare(this,c) == 0){ return true; }
        return false;
    }

  }
/* **************************************************** */

}

Answer 1

Some additions to coderodde's valuable comments:

equals() and hashCode()

In the Channel class, you override the equals() method, but not hashCode(). Then, using Channels in hashtable-based collections (Hashtable, HashMap, HashSet etc.) will give unexpected results.

So, always override both, using the same fields for comparison and for hash value calculation (or let your IDE like Eclipse generate these methods for you).

Channel constructor

It's at least unusual to have a constructor register the newly-created object in some registry outside of the instance (the my_nesteds.add(this); statement). A constructor should only create the instance. It's the responsibility of the code calling the constructor to put this instance into a list or set. What makes things worse is that you add to my_nesteds inside the Channel constructor, but to channels in the addChannel() method.

Iterator

You implement an Iterator yourself, but you could as well just let the channels set do it for you:

@Override
public Iterator<Radio.Channel> iterator() {
    return channels.iterator();
}

The only risk is that a (non-cooperative) caller of that iterator might use the Iterator.remove() method and change your channels list from outside. If you want to protect against that, you can do

@Override
public Iterator<Radio.Channel> iterator() {
    return Collections.unmodifiableSet(channels).iterator();
}

Naming conventions

While most of your names follow the established Java naming conventions, my_nesteds doesn't. Instance fields should be written in "camelCase", meaning your variable should be written myNesteds. The underscore is only used in ALL_CAPITALS constants. By the way, myNesteds is not a good name anyway, something like channelsList would be more helpful.

Regarding English or Italian: that's up to you, but your current code shows a wild mixture of both languages. Of course, if you want to collaborate on an international level (what you're already doing here), English is the way to go (and doesn't seem to be a problem for you).

Javadoc

You should make it a habit to document public classes and methods, those that might be called by someone else when working in a team, so they know what to expect from e.g. calling the nearest() method. Use the Javadoc conventions (probably well supported by your IDE). You can use the wording from the task text as a good example.

Describe what the method does, not how it does it (your version vs. coderodde's version). Describe it from the user's point of view - to the user it doesn't matter whether you use a linear search or some TreeSet methods to find the nearest channel.

This not only helps you when writing your code (and reading it, maybe next year), but also defines the interface, how to use the class. Users should be able to create Radios and Channels without having to read your code, just by looking at the documentation. And if you want to improve your implementation later, you can throw away everything, as long as you still provide the same public classes and methods, and they stille follow the documented behaviour.

Answer 2

Advice 1: missing package

I suggest you put your classes into a package. Consider something like:

package com.github.giuseppe.radio;

imports...

code...

Advice 2

import java.util.*;

I suggest you explicitly list all the classes you import from java.util.

Advice 3

private Set<Channel> channels;
private List<Channel> my_nesteds;

I suggest the following:

private final NavigableSet<Channel> channels = new TreeSet<>();

(See later.) With that arrangement you can get rid of the constructor since you initialize the data structures within field definition.

Advice 4

public Channel addChannel(String nome, double frequenza) {
    Channel new_channel = new Channel(nome,frequenza);
    if(!channels.add(new_channel)){
        throw new RuntimeException("...");
    }
    return new_channel;

}

Allow me to disagree, but I think the following is more idiomatic:

public void addChannel(Channel channel) {
    channels.add(Objects.requireNonNull(channel, "Channel is null."));
}

Advice 5: the nearest channel algorithm

Your current solution will iterate the entire (sorted by frequencies) channel set. You can stop the iteration earlier. (See later.)

Advice 6

protected class Channel{

    private String nome;
    private double frequenza;

    public Channel(String nome, double frequenza) {
        this.nome = nome;
        this.frequenza = frequenza;
        my_nesteds.add(this);
    }

    @Override
    public String toString() {
        return nome+" "+"("+frequenza+")";
    }


    @Override
    public boolean equals(Object o){
        if(!(o instanceof Channel)){ return false; }
        Channel c = (Channel) o;
        if(COMP.compare(this,c) == 0){ return true; }
        return false;
    }

  }

nome, frequenza: I suggest you use English for naming your identifiers. Consider name and frequency instead.

Summa summarum

All in all, I though about this implementation:

radio.Channel:

package radio;

import java.util.Objects;

public class Channel implements Comparable<Channel> {

    private final String name;
    private final double frequency;

    public Channel(String name, double frequency) {
        this.name = checkName(name);
        this.frequency = checkFrequency(frequency);
    }
    
    public double getFrequency() {
        return frequency;
    }
    
    public String getName() {
        return name;
    }

    @Override
    public String toString() {
        StringBuilder sb = new StringBuilder("[Channel: ");
        return sb.append(name)
                 .append(", frequency: ")
                 .append(frequency)
                 .append("]").toString();
    }

    @Override
    public boolean equals(Object o){
        if(!(o instanceof Channel)){ 
            return false; 
        }
        
        Channel c = (Channel) o;
        return frequency == c.frequency;
    }

    private static String checkName(String name) {
        Objects.requireNonNull(name, "The channel name is null.");
        
        if (name.isBlank()) {
            throw new IllegalArgumentException("The channel name is blank.");
        }
        
        return name;
    }
    
    private static double  checkFrequency(double frequency) {
        if (Double.isNaN(frequency)) {
            throw new IllegalArgumentException("The frequency is NaN.");
        }
        
        if (Double.isInfinite(frequency)) {
            throw new IllegalArgumentException(
                    frequency < 0.0 ? 
                            "The frequency is positive infinite." :
                            "The frequency is negative infinite.");
        }
        
        return frequency;
    }

    @Override
    public int compareTo(Channel o) {
        return Double.compare(frequency, o.frequency);
    }
}

radio.Radio:

package radio;

import java.util.Iterator;
import java.util.NavigableSet;
import java.util.NoSuchElementException;
import java.util.Objects;
import java.util.TreeSet;
import java.util.function.Consumer;

public class Radio {

    private final NavigableSet<Channel> channels = new TreeSet<>();

    public void addChannel(Channel channel) {
        channels.add(Objects.requireNonNull(channel, "Channel is null."));
    }

    public Channel getNearest(double frequency) {
        switch (channels.size()) {
            case 0:
                throw new NoSuchElementException("No channels in radio.");

            case 1:
                return channels.first();
        }

        if (frequency <= channels.first().getFrequency()) {
            return channels.first();
        }

        if (frequency >= channels.last().getFrequency()) {
            return channels.last();
        }

        Channel token = new Channel("Token", frequency);
        Channel channelA = channels.floor(token);
        Channel channelB = channels.ceiling(token);

        double frequencyDistanceA = frequency - channelA.getFrequency();
        double frequencyDistanceB = channelB.getFrequency() - frequency;

        return frequencyDistanceA <= frequencyDistanceB ?
                channelA :
                channelB;
    }

    public Iterable<Channel> getChannelsIterable() {
        return new Iterable<>() {

            @Override
            public Iterator<Channel> iterator() {

                return new Iterator<>() {
                    private final Iterator<Channel> iterator = 
                            channels.iterator();

                    @Override
                    public boolean hasNext() {
                        return iterator.hasNext();
                    }

                    @Override
                    public Channel next() {
                        return iterator.next();
                    }

                    @Override
                    public void remove() {
                        throw new UnsupportedOperationException(
                                "No remove allowed.");
                    }

                    @Override
                    public void forEachRemaining(
                            Consumer<? super Channel> action) {
                        throw new UnsupportedOperationException(
                                "No forEachRemaining allowed.");
                    }
                };
            }
        };
    }

    public static void main(String[] args) {
        Radio r = new Radio();
        r.addChannel(new Channel("Rai Radio Uno", 89.3));
        r.addChannel(new Channel("Radio Monte Carlo", 96.4));
        r.addChannel(new Channel("Radio Kiss Kiss", 101.4));

        for (Channel c: r.getChannelsIterable()) {
             System.out.println(c);
        }

        System.out.println(r.getNearest(98.1));
    }
}