VPF::Покритикуйте код - Форум программистов

Absinthe

Дата 22.10.2011, 16:07 (ссылка)

(нет голосов)

Загрузка ...

Опытный

Профиль
Группа: Участник
Сообщений: 526
Регистрация: 4.5.2011

Репутация: нет
Всего: 11

Это моя вторая прога на джаве.
Готов выслушать любую критику кроме "Да это же blob", "Где неймспейсы?" и "Тут нет javadoc".
Конкретнее интересует, правильно ли использую исключения.

Код


import org.apache.http.HttpEntity;
import org.apache.http.HttpResponse;
import org.apache.http.NameValuePair;
import org.apache.http.client.ClientProtocolException;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.client.utils.URLEncodedUtils;
import org.apache.http.impl.client.DefaultHttpClient;
import org.apache.http.message.BasicNameValuePair;
import org.apache.http.util.EntityUtils;
import org.json.simple.parser.JSONParser;
import org.json.simple.parser.ParseException;

import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;

class ApiClientException extends Exception {
}

class MalformedResponseException extends ApiClientException {
}

class NoUserException extends ApiClientException {
}

class AccessDeniedException extends ApiClientException {
}

class IncorrectEmailException extends ApiClientException {
}

public class ApiClient {
    protected String apiUrl;
    JSONParser parser;

    ApiClient(String apiUrl) {
        this.apiUrl = apiUrl;
        parser = new JSONParser();
    }

    protected Object makeRequest(List<NameValuePair> params) throws IOException, MalformedResponseException {
        DefaultHttpClient httpclient = new DefaultHttpClient();
        try {
            String request = URLEncodedUtils.format(params, "UTF-8");
            HttpGet httpget = new HttpGet(apiUrl + "?" + request);
            HttpResponse response = httpclient.execute(httpget);
            HttpEntity entity = response.getEntity();
            if (entity != null) {
                String result = EntityUtils.toString(entity);
                try {
                    return parser.parse(result);
                } catch (ParseException e) {
                    throw new MalformedResponseException();
                }
            }
            EntityUtils.consume(entity);
        } catch (ClientProtocolException e) {
            throw new MalformedResponseException();
        } finally {
            httpclient.getConnectionManager().shutdown();
        }
        return null;
    }

    public String getSalt(String username) throws NoUserException, MalformedResponseException, IOException {
        List<NameValuePair> params = new ArrayList<NameValuePair>();
        params.add(new BasicNameValuePair("api", "getSalt"));
        params.add(new BasicNameValuePair("username", username));
        String response;
        try {
            response = (String) makeRequest(params);
        } catch (ClassCastException e) {
            throw new MalformedResponseException();
        }
        if (response.length() == 0) {
            throw new NoUserException();
        }
        return response;
    }

    public String getToken(String salt, String hash) throws NoUserException, MalformedResponseException, IOException {
        List<NameValuePair> params = new ArrayList<NameValuePair>();
        params.add(new BasicNameValuePair("api", "getToken"));
        params.add(new BasicNameValuePair("salt", salt));
        params.add(new BasicNameValuePair("hash", hash));
        String response;
        try {
            response = (String) makeRequest(params);
        } catch (ClassCastException e) {
            throw new MalformedResponseException();
        }
        if (response.length() == 0) {
            throw new NoUserException();
        }
        return response;
    }

    public Map auth(String token) throws NoUserException, MalformedResponseException, IOException {
        List<NameValuePair> params = new ArrayList<NameValuePair>();
        params.add(new BasicNameValuePair("api", "auth"));
        params.add(new BasicNameValuePair("token", token));
        Map response;
        try {
            response = (Map) makeRequest(params);
        } catch (ClassCastException e) {
            throw new MalformedResponseException();
        }
        if (response == null) {
            throw new NoUserException();
        }
        return response;
    }

    public void userCreate(String token, String newUserName, String newPassword, String newEmail)
            throws MalformedResponseException, IOException, AccessDeniedException, IncorrectEmailException {
        List<NameValuePair> params = new ArrayList<NameValuePair>();
        params.add(new BasicNameValuePair("api", "userCreate"));
        params.add(new BasicNameValuePair("token", token));
        params.add(new BasicNameValuePair("username", newUserName));
        params.add(new BasicNameValuePair("password", newPassword));
        params.add(new BasicNameValuePair("email", newEmail));
        String response;
        try {
            response = (String) makeRequest(params);
        } catch (ClassCastException e) {
            throw new MalformedResponseException();
        }
        if (response.equals("Access denied")) {
            throw new AccessDeniedException();
        }
        if (response.equals("Incorrect Email")) {
            throw new IncorrectEmailException();
        }
        if (!response.equals("OK")) {
            throw new MalformedResponseException();
        }
    }

}

P.S. Где про дектораторы(@something перед функциями) почитать можно? желательно подробно и предпочтительно на русском smile

Это сообщение отредактировал(а) Absinthe - 22.10.2011, 16:20

Stolzen

Дата 22.10.2011, 17:53 (ссылка)

(нет голосов)

Загрузка ...

Эксперт

Профиль
Группа: Завсегдатай
Сообщений: 1041
Регистрация: 17.10.2005

Репутация: 23
Всего: 48

Декораторы - это в питоне, в джаве это называется аннотациями - почитать например тут

Исключения в принципе нормально используются - перехватывать исключения и вместо них бросать свои - это хорошая практика. Однако неплохо было бы не просто бросать исключение наверх, а в него еще оборачивать cause -

Код


                } catch (ParseException e) {
                    throw new MalformedResponseException(e);
                }

Непонятно возвращение null в методе makeRequest, зачем оно?

--------------------

datatalks.ru - анализ данных, статистика, машинное обучение

Absinthe

Дата 22.10.2011, 18:08 (ссылка)

(нет голосов)

Загрузка ...

Опытный

Профиль
Группа: Участник
Сообщений: 526
Регистрация: 4.5.2011

Репутация: нет
Всего: 11

Спасибо.

Цитата

Непонятно возвращение null в методе makeRequest, зачем оно?

Ошибка компиляции: missing return statement

Цитата

catch (ParseException e) {
throw new MalformedResponseException(e);

Так типы же разные? Кастить до Object?

Stolzen

Дата 22.10.2011, 18:11 (ссылка)

(нет голосов)

Загрузка ...

Эксперт

Профиль
Группа: Завсегдатай
Сообщений: 1041
Регистрация: 17.10.2005

Репутация: 23
Всего: 48

Цитата(Absinthe @ 22.10.2011, 19:08

)

Ошибка компиляции: missing return statement

Использовать void

Цитата(Absinthe @ 22.10.2011, 19:08

)

catch (ParseException e) {
throw new MalformedResponseException(e);
Так типы же разные? Кастить до Object?

Не, сделать конструктор, который принимает Throwable - посмотрите реализации Throwable/Exception в исходниках.

Добавлено через 1 минуту и 49 секунд
Кстати HttpClient можно ведь создать один раз, зачем его при каждом запросе создавать? Заодно и Dependency Injection обеспечите

--------------------

datatalks.ru - анализ данных, статистика, машинное обучение

Absinthe

Дата 22.10.2011, 20:45 (ссылка)

(нет голосов)

Загрузка ...

Опытный

Профиль
Группа: Участник
Сообщений: 526
Регистрация: 4.5.2011

Репутация: нет
Всего: 11

Цитата

Использовать void

И передавать значение через поле? Не нравится smile

Потоки данных станут менее понятными.

Цитата

Не, сделать конструктор, который принимает Throwable

Т.е. 2 конструктора: без параметров и Throwable?

Цитата

Кстати HttpClient можно ведь создать один раз, зачем его при каждом запросе создавать? Заодно и Dependency Injection обеспечите

Ок. Просто пытаюсь от этого уйти - мало ли какое состояние они внутри сохраняют.

Это сообщение отредактировал(а) Absinthe - 22.10.2011, 23:52

Absinthe

Дата 23.10.2011, 12:42 (ссылка)

(нет голосов)

Загрузка ...

Опытный

Профиль
Группа: Участник
Сообщений: 526
Регистрация: 4.5.2011

Репутация: нет
Всего: 11

А как хранить эксепшены? Мне пришлось для них пакет создать и в него класть их в 1 файл по 1 эксепшену.
И как правильно файлы хранить? Я создал пакет appName и туда все кроме main кинул.
Как я понимаю, этот пакет лучше назвать org.domain.project, т.е. в IDE он должен лежать в пакете domain в пакете org?

Stolzen

Дата 23.10.2011, 13:40 (ссылка)

(нет голосов)

Загрузка ...

Эксперт

Профиль
Группа: Завсегдатай
Сообщений: 1041
Регистрация: 17.10.2005

Репутация: 23
Всего: 48

Цитата(Absinthe @ 22.10.2011, 21:45

)

И передавать значение через поле? Не нравится smile Потоки данных станут менее понятными.

Ну все равно, я бы отрефакторил этот метод. Он возвращает Object, потом вы делаете приведение типов, ловите ClassCastExpeption... много лишнего, вообщем.

Цитата(Absinthe @ 23.10.2011, 13:42

)

А как хранить эксепшены? Мне пришлось для них пакет создать и в него класть их в 1 файл по 1 эксепшену.

Да, это нормально. После питона/руби/(другой язык) кажется черезмерным, но постепенно привыкаешь.

--------------------

datatalks.ru - анализ данных, статистика, машинное обучение

0 Пользователей читают эту тему (0 Гостей и 0 Скрытых Пользователей)
0 Пользователей:
« Предыдущая тема \| Java: Общие вопросы \| Следующая тема »