Diskussion INI-Dateien parsen und generieren - Codereview

Mat

Mat

Aktives Mitglied
Und wieder mal Gammelcode.

Die Aufgabe lautete, einen Mailproxy zu schreiben, der Nachrichten von einem Server abruft und in seiner Inbox zur Verfügung stellt. Es ist aber zu viel Code, um den hier reinzustellen und die einzelnen Dateien ohne Kontext zu betrachten wäre auch etwas schwierig.

Stattdessen habe ich einen Teil des Codes isoliert, der die Servereinstellungen und Accounts über INI-Dateien auslesen sollte (um das Testen zu vereinfachen). Und der Code ist schon lang genug:

IniHelper: 
package proxy.util;

import proxy.model.Account;
import proxy.model.MailServer;

import java.io.*;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

/**
 * Helps with parsing and generating INI configurations for the mail proxy.
 * Server settings support "x = y" pairs,
 * Accounts support sections followed by "x = y" pairs.
 * Comments like "# text" are not supported
 */
public final class IniHelper {

    private static final Pattern PATTERN_SECTION = Pattern.compile("\\s*\\[(?<section>[^]]*)]\\s*");
    private static final Pattern PATTERN_SETTING = Pattern.compile("\\s*(?<key>[^=]*)=(?<value>.*)");

    private static final File FILE_CONFIG = new File("./config.ini");
    private static final File FILE_ACCOUNTS = new File("./accounts.ini");

    private static final Map<ServerConfig, Object> SETTINGS_DEFAULT = new HashMap<>();

    static {
        SETTINGS_DEFAULT.put(ServerConfig.PORT, ServerConfig.PORT.getDefaultValue());
        SETTINGS_DEFAULT.put(ServerConfig.INTERVAL_IN_SEC, ServerConfig.INTERVAL_IN_SEC.getDefaultValue());
        SETTINGS_DEFAULT.put(ServerConfig.CLIENT_TIMEOUT, ServerConfig.CLIENT_TIMEOUT.getDefaultValue());
        SETTINGS_DEFAULT.put(ServerConfig.MAX_CLIENTS, ServerConfig.MAX_CLIENTS.getDefaultValue());
    }

    private static final Map<String, Map<AccountConfig, Object>> ACCOUNTS_DEFAULT = new HashMap<>();

    static {
        for (int i = 1; i < 3; i++) {
            Map<AccountConfig, Object> defaultUser = new HashMap<>();
            defaultUser.put(AccountConfig.USER, AccountConfig.USER.getDefaultValue());
            defaultUser.put(AccountConfig.PASS, AccountConfig.PASS.getDefaultValue());
            defaultUser.put(AccountConfig.SERVER, AccountConfig.SERVER.getDefaultValue());
            defaultUser.put(AccountConfig.SMTP, AccountConfig.SMTP.getDefaultValue());
            defaultUser.put(AccountConfig.POP3, AccountConfig.POP3.getDefaultValue());

            ACCOUNTS_DEFAULT.put(String.format("user-%d", i), defaultUser);
        }
    }

    public static Map<ServerConfig, Object> getOrGenerateConfig() throws IOException, ServerExceptions.UnknownParameter, ServerExceptions.InvalidParameter {
        if (FILE_CONFIG.exists()) {
            final Map<ServerConfig, Object> settings = new HashMap<>();

            try (BufferedReader reader = new BufferedReader(new FileReader(FILE_CONFIG))) {
                String line;
                while ((line = reader.readLine()) != null) {
                    String[] kvPair = line.split(ServerConfig.DELIMITER, 2);

                    String key = kvPair[0].trim();
                    String val = kvPair.length == 2 ? kvPair[1].trim() : null;

                    ServerConfig paramEnum;
                    try {
                        paramEnum = ServerConfig.valueOf(key);
                    } catch (IllegalArgumentException ex) {
                        throw new ServerExceptions.UnknownParameter();
                    }

                    if (!paramEnum.isValidParam(val)) throw new ServerExceptions.InvalidParameter(paramEnum.getName());

                    settings.put(paramEnum, val);
                }
            }
            if (!settings.values().isEmpty()) return settings;
        }

        System.err.printf("%s not found, will be generated ...\n", FILE_CONFIG.getAbsolutePath());
        if (FILE_CONFIG.createNewFile()) {
            try (BufferedWriter writer = new BufferedWriter(new FileWriter(FILE_CONFIG))) {
                for (Map.Entry<ServerConfig, Object> setting : SETTINGS_DEFAULT.entrySet()) {
                    writer.append(String.format("%s = %s", setting.getKey().getName(), setting.getValue()));
                    writer.newLine();
                }
            }
        } else {
            throw new IOException("Could not create file!");
        }
        return SETTINGS_DEFAULT;
    }

    public static Map<String, Map<AccountConfig, Object>> getOrGenerateAccounts() throws IOException, ServerExceptions.UnknownParameter, ServerExceptions.InvalidParameter {
        if (FILE_ACCOUNTS.exists()) {
            final Map<String, Map<AccountConfig, Object>> accounts = new HashMap<>();

            try (BufferedReader reader = new BufferedReader(new FileReader(FILE_ACCOUNTS))) {

                String line = null;
                String sectionName = null;
                while ((line = reader.readLine()) != null) {
                    Matcher m = PATTERN_SECTION.matcher(line);

                    if (m.matches()) {
                        sectionName = m.group("section");
                    } else if (sectionName != null) {
                        m = PATTERN_SETTING.matcher(line);

                        if (m.matches()) {
                            String key = m.group("key").trim();
                            String value = m.group("value").trim();
                            Map<AccountConfig, Object> pairs = accounts.computeIfAbsent(sectionName, k -> new HashMap<>());

                            AccountConfig accountEnum;
                            try {
                                accountEnum = AccountConfig.valueOf(key);
                            } catch (IllegalArgumentException ex) {
                                throw new ServerExceptions.UnknownParameter();
                            }

                            if (!accountEnum.validParam(value))
                                throw new ServerExceptions.InvalidParameter(accountEnum.getName());

                            pairs.put(accountEnum, value);
                        }
                    }
                }
            }

            if (!accounts.values().isEmpty()) {
                return accounts;
            } else {
                System.err.printf("%s exists but contains no accounts. Delete it to generate a default one.\n", FILE_ACCOUNTS.getAbsolutePath());
                System.exit(-1);
            }
        }

        System.err.printf("%s not found, creating default accounts file ...\n", FILE_ACCOUNTS.getAbsolutePath());
        if (FILE_ACCOUNTS.createNewFile()) {
            try (BufferedWriter writer = new BufferedWriter(new FileWriter(FILE_ACCOUNTS))) {
                for (Map.Entry<String, Map<AccountConfig, Object>> section : ACCOUNTS_DEFAULT.entrySet()) {
                    writer.append(String.format("[%s]", section.getKey()));
                    writer.newLine();
                    for (Map.Entry<AccountConfig, Object> account : section.getValue().entrySet()) {
                        writer.append(String.format("%s = %s", account.getKey().getName(), account.getValue()));
                        writer.newLine();
                    }
                    writer.newLine();
                }
            }
            System.err.printf("%s with default accounts created. Edit the values before starting the server.\n", FILE_ACCOUNTS.getAbsolutePath());
            System.exit(-1);
        } else {
            throw new IOException("Could not create file!");
        }

        return ACCOUNTS_DEFAULT;
    }

    public static List<Account> getAccountsListFromMap(Map<String, Map<AccountConfig, Object>> map) {
        List<Account> accounts = new ArrayList<>();

        for (String accId : map.keySet()) {
            String user = String.valueOf(map.get(accId).get(AccountConfig.USER));
            String password = String.valueOf(map.get(accId).get(AccountConfig.PASS));
            String servername = String.valueOf(map.get(accId).get(AccountConfig.SERVER));
            String portPOP3Text = String.valueOf(map.get(accId).get(AccountConfig.POP3));
            String portSMTPText = String.valueOf(map.get(accId).get(AccountConfig.SMTP));

            int portPOP3 = 0;
            int portSMTP = 0;
            try {
                if (user == null || password == null || servername == null || portPOP3Text == null || portSMTPText == null) {
                    throw new Exception();
                }
                portPOP3 = Integer.parseUnsignedInt(portPOP3Text);
                portSMTP = Integer.parseUnsignedInt(portSMTPText);
            } catch (Exception n) {
                System.err.printf("Incomplete or invalid data. Please edit values for %s delete the entry.\n", accId);
                System.exit(-1);
            }

            Account account = new Account(accId, new MailServer(portPOP3, portSMTP, servername), user, password);
            accounts.remove(account);
            accounts.add(account);
        }

        return accounts;
    }
}

Enums
AccountConfig: 
package proxy.util;

public enum AccountConfig {

    USER("USER", String.class, "user@localhost"),
    PASS("PASS", String.class, "password"),
    SERVER("SERVER", String.class, "srv1"),
    SMTP("SMTP", Integer.class, 12345),
    POP3("POP3", Integer.class, 23456);

    private final String name;
    private final Class paramType;
    private final Object defaultValue;

    Class getParamType() {
        return paramType;
    }

    String getName() {
        return name;
    }

    public Object getDefaultValue() {
        return defaultValue;
    }

    public boolean validParam(String parameter) {
        if (parameter == null) return paramType == null;

        if (getParamType().equals(Integer.class)) {
            try {
                return Integer.parseInt(parameter) > 0;
            } catch (NumberFormatException e) { /* _ */}
        }

        if (getParamType().equals(String.class)) {
            return !parameter.isEmpty();
        }

        return false;
    }

    AccountConfig(String name, Class paramType, Object defaultValue) {
        this.name = name;
        this.paramType = paramType;
        this.defaultValue = defaultValue;
    }

}

ServerConfig: 
package proxy.util;

public enum ServerConfig {

    PORT("PORT", Integer.class, 55555),
    INTERVAL_IN_SEC("INTERVAL_IN_SEC", Integer.class, 30),
    CLIENT_TIMEOUT("CLIENT_TIMEOUT", Integer.class, 180),
    MAX_CLIENTS("MAX_CLIENTS", Integer.class, 3);

    public static final String DELIMITER = "=";

    private final String name;
    private final Class paramType;
    private final Object defaultValue;

    Class getParamType() {
        return paramType;
    }

    String getName() {
        return name;
    }

    public Object getDefaultValue() {
        return defaultValue;
    }

    public boolean isValidParam(String param) {
        if (param == null) return paramType == null;

        if (getParamType().equals(Integer.class)) {
            try {
                return Integer.parseInt(param) > 0;
            } catch (NumberFormatException e) { /* _ */ }
        }

        return false;
    }

    ServerConfig(String name, Class paramType, Object defaultValue) {
        this.name = name;
        this.paramType = paramType;
        this.defaultValue = defaultValue;
    }

}

Entitäten
Account: 
package proxy.model;

public final class Account {

    private final String accountId;
    private final String username;
    private final String password;
    private final MailServer mailServer;

    public Account(String accountId, MailServer mailServer, String username, String password) {
        this.accountId = accountId;
        this.mailServer = mailServer;
        this.username = username;
        this.password = password;
    }

    public String getAccountId() {
        return accountId;
    }

    @Override
    public boolean equals(Object other) {
        if (other == null) return false;
        if (other instanceof Account) return ((Account) other).getAccountId().equals(this.getAccountId());
        return false;
    }

    @Override
    public String toString() {
        return String.format("id: %s; name: %s; server: { %s }", accountId, username, mailServer);
    }
}

MailServer: 
package proxy.model;

public final class MailServer {
    private final int portPOP3;
    private final int portSMTP;
    private final String address;

    public MailServer(int portPOP3, int portSMTP, String address) {
        this.portPOP3 = portPOP3;
        this.portSMTP = portSMTP;
        this.address = address;
    }

    @Override
    public String toString() {
        return String.format("POP3: %d; SMTP: %d; address: %s", portPOP3, portSMTP, address);
    }
}

Sonstige
ServerExceptions: 
package proxy.util;

public class ServerExceptions extends Exception {

    private ServerExceptions() { /* _ */ }

    private ServerExceptions(String msg) { super("-ERR " + msg);}

    public static final class SyntaxError extends ServerExceptions {
        public SyntaxError(String msg) {super(msg);}
    }

    public static final class UnknownCommand extends ServerExceptions {
        public UnknownCommand() {super("COMMAND unknown");}
    }

    public static final class UnknownParameter extends ServerExceptions {
        public UnknownParameter() {super("PARAM unknown");}
    }

    public static final class InvalidParameter extends ServerExceptions {
        public InvalidParameter(String msg) { super("PARAM invalid for " + msg);}
    }

    public static final class WrongState extends ServerExceptions {
        public WrongState(String msg) { super("STATE " + msg);}
    }

    public static class Unauthorized extends ServerExceptions {
        public Unauthorized(String msg) { super("AUTH " + msg); }
    }

    public static final class POP3Exception extends ServerExceptions {
        public POP3Exception() {this("POP3 ERR received from remote server"); }

        public POP3Exception(String msg) { super(msg); }
    }
}

Aufruf und generierte Dateien

Beispielaufruf ohne Exceptionhandling

Java: 
public static void main(String[] args) throws Exception {
    IniHelper.getOrGenerateConfig();
    IniHelper.getOrGenerateAccounts();
    IniHelper.getAccountsListFromMap(IniHelper.getOrGenerateAccounts()).forEach(System.out:println);
}

Ausgabe
id: user-1; name: user@localhost; server: { POP3: 23456; SMTP: 12345; address: srv1 }
id: user-2; name: user@localhost; server: { POP3: 23456; SMTP: 12345; address: srv1 }

Generiert

accounts.ini: 
[user-1]
PASS = password
SMTP = 12345
POP3 = 23456
SERVER = srv1
USER = user@localhost

[user-2]
PASS = password
SMTP = 12345
POP3 = 23456
SERVER = srv1
USER = user@localhost

config.ini: 
PORT = 55555
CLIENT_TIMEOUT = 180
INTERVAL_IN_SEC = 30
MAX_CLIENTS = 3

Meine Gedanken
  • uiuiui.. einfach alles in die INI-Klasse geworfen.. dadurch kann man sie nur für diesen speziellen Fall verwenden, anstatt für alle INIs generell
  • wenn ich das nochmal machen müsste, dann würde ich versuchen, eine "dumme" Ini-Klasse zu machen, die intern nur mit LinkedHashMaps und Strings arbeitet, und dann nach außen hin Funktionen für das Generieren und Parsen von INIs anbieten (oder einfach eine Library importieren, die das schon macht)
  • zu viele Exception-Weiterleitungen.. die wären in den Config-Klassen besser aufgehoben, denke ich
  • wenn ich Refactoring machen würde, dann würde ich wohl erstmal Teile der INI-Klasse in die Config-Klassen verschieben und dann weiterschauen
  • eine gesonderte Klasse zum Lesen und Schreiben der Dateien wäre auch ganz gut
  • für diesen Fall sind Enums ganz OK, denke ich, weil nur bestimmte Werte in der Konfiguration akzeptiert werden
  • insgesamt aber schlecht testbar und nicht sehr übersichtlich (habe schon ganz viel rausgelöscht, um es teilen zu können)
 
Meiner Meinung nach trifft es deine Liste mit eigenen Gedanken schon ziemlich gut. Mir sind zwei Dinge aufgefallen.

Ich würde solche Sachen hier vermeiden:

IniHelper: 
private static final File FILE_CONFIG = new File("./config.ini");
private static final File FILE_ACCOUNTS = new File("./accounts.ini");

  • Die Dateien werden über die gesamte Laufzeit des Mailproxies im Speicher gehalten
  • An dieser Stelle ist kein Handling der NullPointerException möglich
  • Es ist unklar, was an welcher Stelle des Programms mit dem Objekt gemacht wird
Der zweite Punkt ist einfach, dass die IniHelper-Klasse im Grunde genommen (wie du schon sagtest) alles macht, es gibt keine Single Responsibility und keine Separation of Concerns. Dementsprechend schlecht ist sind eben Testbarkeit und Erweiterbarkeit.
 
Zurück
Oben Unten