Версия для печати темы
Нажмите сюда для просмотра этой темы в оригинальном формате
Форум программистов > Java: Общие вопросы > Покритикуйте код


Автор: Absinthe 22.10.2011, 16:07
Это моя вторая прога на джаве.
Готов выслушать любую критику кроме "Да это же 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

Автор: Stolzen 22.10.2011, 17:53
Декораторы - это в питоне, в джаве это называется аннотациями - почитать например http://www.javenue.info/post/79

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

Код

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


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

Автор: Absinthe 22.10.2011, 18:08
Спасибо.

Цитата

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

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

Цитата

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

Автор: Stolzen 22.10.2011, 18:11
Цитата(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 обеспечите

Автор: Absinthe 22.10.2011, 20:45
Цитата

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

Цитата

Не, сделать конструктор, который принимает Throwable
 Т.е. 2 конструктора: без параметров и Throwable?

Цитата

Кстати HttpClient можно ведь создать один раз, зачем его при каждом запросе создавать? Заодно и Dependency Injection обеспечите
 Ок. Просто пытаюсь от этого уйти - мало ли какое состояние они внутри сохраняют.

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

Автор: Stolzen 23.10.2011, 13:40
Цитата(Absinthe @  22.10.2011,  21:45 Найти цитируемый пост)
 И передавать значение через поле? Не нравится smile Потоки данных станут менее понятными.

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

Цитата(Absinthe @  23.10.2011,  13:42 Найти цитируемый пост)
А как хранить эксепшены? Мне пришлось для них пакет создать и в него класть их в 1 файл по 1 эксепшену.

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

Powered by Invision Power Board (http://www.invisionboard.com)
© Invision Power Services (http://www.invisionpower.com)