Esta abordagem está correta?

Para deixar generico da para fazer esta modificação

public List<OS> getOsByStatus(SuperclasseOuInterfaceQueTenhaGetCod o){   
    List<OS> retorno = new ArrayList<OS>();   
    long codAgente = 0;   
    String sql = "";   
       
    sql = " SELECT * FROM os WHERE osagetec = ? ";   
    codAgente = tec.getCod(); 
       
    PreparedStatement stmt;   
    ResultSet rs;   
    try {   
        stmt = getConn().prepareStatement(sql);   
        stmt.setLong(1, codAgente);   
        rs = stmt.executeQuery();   
           
        while (rs.next()) {   
            // Popula o List   
        }   
           
    }catch(SQLException e) {   
        e.printStackTrace();   
    }   
    return retorno;   
}

1)Vc não esta fechando a conexão com o banco de dados, CUIDADO

A minha sugestão não é das melhores pois pensando um tecnico não é um cliente, não tem prq especializa-lo, fazendo um superclasse\interface.
Eu não deixaria generico.
No seu lugar eu faria:

public List<OS> getOsByStatus(Tecnico o){   
    List<OS> retorno = new ArrayList<OS>();   
    long codAgente = 0;   
    String sql = "";   
       
        sql = " SELECT * FROM os WHERE osagetec = ? ";   
        codAgente = o.getCod();   
       
    PreparedStatement stmt;   
    ResultSet rs;   
    try {   
        stmt = getConn().prepareStatement(sql);   
        stmt.setLong(1, codAgente);   
        rs = stmt.executeQuery();   
           
        while (rs.next()) {   
            // Popula o List   
        }   
           
    }catch(SQLException e) {   
        e.printStackTrace();   
    } finally {
        //fecha as conexões
    }   
    return retorno;   
}

O mesmo para o outro metodo.

Abacate:

Para deixar generico da para fazer esta modificação

1)Vc não esta fechando a conexão com o banco de dados, CUIDADO

Não esquenta. Ainda está em desenvolvimento. Ainda tenho que fazer todos os sets dentro do while! =)
E outra, estou testando um pool de conexões manual, para ver como se comporta. Na verdade, o que está faltando é:

liberaConexao(getConn());

sql  = null;
stmt = null;

Mesmo assim, vlw pela dica!

Assim, a melhor abordagem seria com um método para cada tipo de agente? Mesmo com tanta repetição de código? Pois, na verdade, os agentes são diferentes, mas as linhas que mudariam seriam a definição do select e código a ser passado. O resto seria exatamente igual.

Muito obrigado pela resposta!

celso.martins:

Fazer este método desta forma está, conceitualmente, correto ou tenho que separar essa lógica em mais métodos? Se eu tiver que separar, 80% da implementação do método será igual. Por isso pensei em fazer dessa forma.

Abstraiam o fato de eu estar usando JDBC. =)

Olha eu perturbando com dúvidas sobre boas práticas novamente.

public List<OS> getOsByStatus(Object o){
		List<OS> retorno = new ArrayList<OS>();
		long codAgente = 0;
		String sql = "";
		
		if (o instanceof Tecnico) {
			sql = " SELECT * FROM os WHERE osagetec = ? ";
			Tecnico tec = (Tecnico) o;
			codAgente = tec.getCod();
		}else if (o instanceof Cliente){
			sql = " SELECT * FROM os WHERE osagecli = ? ";
			Cliente c = (Cliente) o;
			codAgente = c.getCod();
		}

...

1. Se é getByStatus proque é que é c.get[b]Cod/b ?
   Deveria ser getByCode ou getStatus ( getCod significa = get “Bacalhau” )

2. Se a sua duvida é em relação a tornar a função mais especifica e não usar Object então vc tem razão. User Object é uma má ideia. Vc de usar uma super classe ou interface.
   Mas já que o select tb é diferente vc pode sim criar dois metodos . Esse metodo obtem o codigo correto e a frase correcta e envia para um metodo utilitário que usa o JDBC para recuperar a lista.

O que acontece é o seguinte. Pelo que pensei no sistema, não faz sentido eu pegar todas as OSs do banco, se a listagem não for específica de um técnico ou de um cliente ou de um gerente.

Com relação ao status é fácil de resolver. Assim que eu tiver resolvido essa “pendenga” o select vai ficar assim:

ou

Blz. A minha dúvida era exatamente essa. Se mesmo repetindo boa parte do código, pelo fato do select ser diferente, faria sentido eu criar um método para cada tipo de agente que precisa interagir com um listagem de OS.

Eu não sabia se bacalhau era isso que escrevi acima ou se era a solução que eu tinha escolhido.

De acordo com a sua resposta, o bacalhau é o meu código. =)

Não tem problema, reescrevo esse método e crio os outros. Acho que na verdade é até mais fácil trabalhar dessa forma. =)

Obrigado pelos esclarecimentos!!

Normalmente considera-se que usar instanceof é gambiarra se poder ser feito de outra forma.

Ou vc chama de getCode , getCodigo ou de getStatus. getCod não significa nada. “Cod” é a abreviação de “Codfish” que é inglês para “Bacalhau”. Se vc é preocupado com boas práticas: escrever os nomes de forma correta e autoexplicativa é uma boa prática.

sergiotaborda:

Ou vc chama de getCode , getCodigo ou de getStatus. getCod não significa nada. “Cod” é a abreviação de “Codfish” que é inglês para “Bacalhau”. Se vc é preocupado com boas práticas: escrever os nomes de forma correta e autoexplicativa é uma boa prática.

Opa! Também não sabia disso. Sempre usei cod como abreviação de Código. Inclusive nos campos da base de dados.

É claro que estou preocupado, e muito, com boas práticas. Normalmente escrevo os nomes de forma autoexplicativa. E pensava que cod se enquadrava em nomes autoexplicativos.

Abraços!

Auhauhauhahu!!! Muito boa!!!
P.S.: Pra quem não sacou, Cod = Bacalhau em inglês… ou seja CodE = Bacalhau eletronico… que fede e não tem pé nem cabeça!!! auhauhauh

Porque não usar polimorfismo?!?!?

Sim, a minha dúvida era se tanta repetição de código não seria uma má prática!

Vlw!

Por favor, vejam se o caminho é esse.

Obrigado

public List<OS> getOsByStatus(Tecnico tecnico){
		List<OS> retorno = new ArrayList<OS>();

		String sql = " SELECT * FROM os WHERE osagetec = ? ";
		
		PreparedStatement stmt;
		ResultSet rs;
		try {
			stmt = getConn().prepareStatement(sql);
			stmt.setLong(1, tecnico.getCod());
			rs = stmt.executeQuery();
			retorno = populaListaOs(rs);
		}catch(SQLException e) {
			e.printStackTrace();
		}

		liberaConexao(getConn());
		rs = null;
		stmt = null;
		
		return retorno;
	}
	
	public List<OS> getOsByStatus(Cliente cliente){
		List<OS> retorno = new ArrayList<OS>();
		
		String sql = " SELECT * FROM os WHERE osagecli = ? ";
		
		PreparedStatement stmt;
		ResultSet rs;
		try {
			stmt = getConn().prepareStatement(sql);
			stmt.setLong(1, cliente.getCod());
			rs = stmt.executeQuery();
			retorno = populaListaOs(rs);
		}catch(SQLException e) {
			e.printStackTrace();
		}

		liberaConexao(getConn());
		rs = null;
		stmt = null;
		
		return retorno;
	}
	
	public List<OS> populaListaOs(ResultSet rs){
		List<OS> retorno = new ArrayList<OS>();
		
		try {
			while (rs.next()) {
				OS os = new OS();
				os.setNumero(rs.getString("osnum"));
				os.setDataAbertura(rs.getDate("osdatabe"));
				os.setDataFechamento(rs.getDate("osdatpronto"));
				os.setNumeroSerie(rs.getString("osns"));
				os.setPatrimonio(rs.getString("ospatrimonio"));
				os.setNotaFiscal(rs.getString("osnf"));
				os.setAcessorios(rs.getString("osace"));
				os.setStatus(rs.getInt("ossta"));
				os.setReclamacao(rs.getString("osinf"));
				os.setConstatado(rs.getString("osdetectado"));
				os.setSolucao(rs.getString("ossolucao"));
				os.setObs(rs.getString("osobs"));
				os.setGarantia(rs.getInt("osgarantia"));
				os.setEmailInformativo(getEmailInformativo(os.getCod()));
				retorno.add(os);
				os = null;
			}
		}catch (SQLException e) {
			e.printStackTrace();
		}
		
		return retorno;
	}

celso.martins:

Por favor, vejam se o caminho é esse.

Vc pode ser mais resumido.

public List<OS> getOsByStatus(Tecnico tecnico){
             return populaListaOs(tecnico.getStatus() ,  " SELECT * FROM os WHERE osagetec = ? " );
	}
	
	public List<OS> getOsByStatus(Cliente cliente){
             return populaListaOs(cliente.getStatus() ,  " SELECT * FROM os WHERE osagecli = ? " );
	}
	
	private List<OS> populaListaOs(long status, String sql){

		PreparedStatement stmt;
		ResultSet rs;
                Connection con = getConn();
		try {
			stmt = con.prepareStatement(sql);
			stmt.setLong(1, status);
			rs = stmt.executeQuery();

			while (rs.next()) {
				OS os = new OS();
				os.setNumero(rs.getString("osnum"));
				os.setDataAbertura(rs.getDate("osdatabe"));
				os.setDataFechamento(rs.getDate("osdatpronto"));
				os.setNumeroSerie(rs.getString("osns"));
				os.setPatrimonio(rs.getString("ospatrimonio"));
				os.setNotaFiscal(rs.getString("osnf"));
				os.setAcessorios(rs.getString("osace"));
				os.setStatus(rs.getInt("ossta"));
				os.setReclamacao(rs.getString("osinf"));
				os.setConstatado(rs.getString("osdetectado"));
				os.setSolucao(rs.getString("ossolucao"));
				os.setObs(rs.getString("osobs"));
				os.setGarantia(rs.getInt("osgarantia"));
				os.setEmailInformativo(getEmailInformativo(os.getCod()));
				retorno.add(os);
			}

		}catch (SQLException e) {
			throw new DataAccessException(e.getMessage());
		} finally {
		       liberaConexao(con);
                }
		
		return retorno;
	}

Meu amigo! Muito obrigado pelos teus esclarecimentos. Está sendo um fds de muito trabalho. Não sei porque, estou com uma vontade danada de codificar.

Outra dúvida que surgiu é se eu teria que mover este método “populaOs” para uma classe utilitária. Depois de pensar no assunto, eu acreditei que não. Se alguém não concordar pode gritar.

O sistema está seguindo o seu curso.

Se eu esbarrar novamente em alguma dúvida de boas práticas, volto a perturbar.

Obs.: Já alterei em todas as classes o cod para id. Só falta no MER.

Segue a refatoração dos métodos:

public List<OS> getOsByStatus(Tecnico tecnico, int status){
		String sql = " SELECT * FROM os WHERE osagetec = ? AND ossta = ? " +
			" ORDER BY osdatabe DESC ";
		return populaListaOs(sql, status, tecnico.getId());
	}
	
	public List<OS> getOsByStatus(Cliente cliente, int status){
		String sql = " SELECT * FROM os WHERE osagecli = ? AND ossta = ? " +
			" ORDER BY osdatabe DESC ";
		return populaListaOs(sql, status, cliente.getId());
	}
	
	private List<OS> populaListaOs(String sql, int status, long codAgente){
		List<OS> retorno = new ArrayList<OS>();
		
		try {
			PreparedStatement stmt = getConn().prepareStatement(sql);;
			stmt.setLong(1, codAgente);
			stmt.setInt(2, status);

			ResultSet rs = stmt.executeQuery();

			while (rs.next()) {
				OS os = new OS();
				os.setId(rs.getLong("oscod"));
				
				TecnicoDAO daoTec = new TecnicoDAO();
				os.setTecnico(daoTec.getTecnicoById(rs.getLong("osagetec")));
				
				ClienteDAO daoCli = new ClienteDAO();
				os.setCliente(daoCli.getClienteById(rs.getLong("osagecli")));
				
				os.setNumero(rs.getString("osnum"));
				os.setDataAbertura(rs.getDate("osdatabe"));
				os.setDataFechamento(rs.getDate("osdatpronto"));
				os.setNumeroSerie(rs.getString("osns"));
				os.setPatrimonio(rs.getString("ospatrimonio"));
				os.setNotaFiscal(rs.getString("osnf"));
				os.setAcessorios(rs.getString("osace"));
				os.setStatus(rs.getInt("ossta"));
				os.setReclamacao(rs.getString("osinf"));
				os.setConstatado(rs.getString("osdetectado"));
				os.setSolucao(rs.getString("ossolucao"));
				os.setObs(rs.getString("osobs"));
				os.setGarantia(rs.getInt("osgarantia"));
				
				// Recupera os emails informativos
				EmailInformativoDAO dao = new EmailInformativoDAO();
				os.setEmailInformativo(dao.getEmailInformativo(os.getId()));
				dao = null;
				
				retorno.add(os);
				os = null;
			}
		}catch (SQLException e) {
			e.printStackTrace();
		}finally {
			liberaConexao(getConn());
		}
		
		return retorno;
	}

Valeu e muito!

Abraços!

Por que não criar métodos distintos?Afinal de contas obter um tecnico, não é o mesmo que obter um agente.
Afinal de contas com isso você está criando um big método que se amanhã aparecer novos tipos de funcionarios(eu acho que é isso que você busca?) você vai aumentar o método de uma forma que depois nem vai saber para que ele serve, porque ele vai virar um verdadeiro faz tudo.
Eu particulamente não acho legal esses métodos faz tudo.
Você pode abstrair o obter conexão com o banco,mas entre fazer um método que tem 20,30,40 linhas e outros que vc tem 6 linhas, é muito mais elegante cada método seu ter 6 linhas.
Na minha opnião.

Já que estamos falando de boas práticas:

1 - Por favor, não use SELECT * FROM TABELA, seleciona os campos que você deseja, isso causa uma carga desnecessária na rede e no banco de dados.

Ex: SELECT CAMPO1, CAMPO3, CAMPO35 FROM TABELA.

2 - Não use nunca e.printStackTrace(); Use o seu gerenciador de logs preferido.

Este código seria substituido por:

LOGGER.error(“Ocorreu um erro no metodo x”, e);

Com essa abordagem vc ainda poderia visualizar o log, apenas setando seu log(log4j?) para SYSOUT.

3 - Evite nomear suas classes como ClasseDAO ou ClasseEJB ou ClasseAction, distribua suas classes em pacotes com as respectivas funcionalidades.

Ex: br.com.minhaempresa.negocio.dao

4 - Crie os campos de sua tabela de uma forma razoavel, se a tabela se chama OS vc nao precisa nomear as colunas como OSCAMPO1, OSCAMPO2, etc. Isso é confuso e desnecessário.

Tem mais algumas coisas, porém já está na minha hora

antoniopopete:

Por que não criar métodos distintos?Afinal de contas obter um tecnico, não é o mesmo que obter um agente.
Afinal de contas com isso você está criando um big método que se amanhã aparecer novos tipos de funcionarios(eu acho que é isso que você busca?) você vai aumentar o método de uma forma que depois nem vai saber para que ele serve, porque ele vai virar um verdadeiro faz tudo.
Eu particulamente não acho legal esses métodos faz tudo.
Você pode abstrair o obter conexão com o banco,mas entre fazer um método que tem 20,30,40 linhas e outros que vc tem 6 linhas, é muito mais elegante cada método seu ter 6 linhas.
Na minha opnião.

Obrigado pelas considerações. =)

Mas os métodos ficaram bem pequenos. Um pouco maior ficou o “populator tabajara”. Talvez seja uma boa razão para usar o Hibernate.

Se entrar outro tipo de funcionário que precise ter permissões diferentes das que já existe, já era e vai ter que ser criado um método novo, uma FK nova na tabela OS. Se puder ser encaixado nesses grupos de funcionários, maravilha. Não sei se existe outra forma de se fazer isso. Ter duas FKs da mesma tabela não me cheira bem. E toda ferramenta de MER que uso ou me dá um warning ou um erro. Não sei mesmo.

Um abraço.

aleck:

Já que estamos falando de boas práticas:

1 - Por favor, não use SELECT * FROM TABELA, seleciona os campos que você deseja, isso causa uma carga desnecessária na rede e no banco de dados.

Ex: SELECT CAMPO1, CAMPO3, CAMPO35 FROM TABELA.

Isso é muito importante. Preciso realmente me aprofundar nesse assunto.

Estou aprendendo agora esses padrões. Custa nada uma cola para ajudar a identificar. E acho que evita algo como chamadas de pacote dentro de métodos. Acho isso meio esquisito. =)

Aprendi isso há uns 4 anos atrás. Me parece que isso é um padrão de anotação de algum pais maluco do leste europeu. Achei que facilita quando se usa JOIN. Sempre achei confuso, mesmo com alias, ficar chamando campos de nomes iguais. Mas tá valendo.

aleck:

Tem mais algumas coisas, porém já está na minha hora :)

Quando puder mande essas outras coisas, por favor.

Obrigado por essas!

Abraços