開発者がチーム内のアプリケーションでより多くの作業を行い、自分のコードを熟知すればするほど、彼は仲間の仕事の校正に頻繁に従事します。 今日は、非常に優秀な開発者によって書かれたコードで、1週間で何をキャッチできるかを示します。 カットの下には、私たちの創造性（および私の考えのいくつか）の鮮やかなアーティファクトのコレクションがあります。
コンパレータ
コードがあります：
@Getter class Dto { private Long id; } // another class List<Long> readSortedIds(List<Dto> list) { List<Long> ids = list.stream().map(Dto::getId).collect(Collectors.toList()); ids.sort(new Comparator<Long>() { public int compare(Long o1, Long o2) { if (o1 < o2) return -1; if (o1 > o2) return 1; return 0; } }); return ids; }
ストリームを直接並べ替えることができると誰かが指摘していますが、コンパレータに注意を向けたいと思います。 Comparator::compare
メソッドのドキュメントには、英語で白で書かれています：
2つの引数の順序を比較します。 最初の引数が2番目の引数より小さい、等しい、または大きいため、負の整数、ゼロ、または正の整数を返します。
この動作がコードに実装されています。 何が悪いの？ 事実、Javaの作成者は、多くの人がそのようなコンパレーターを必要とすることを非常に先見の明をもって提案し、それを私たちのために作りました。 コードを単純化することで使用できます。
List<Long> readSortedIds(List<Dto> list) { List<Long> ids = list.stream().map(Dto::getId).collect(Collectors.toList()); ids.sort(Comparator.naturalOrder()); return ids; }
同様に、このコード
List<Dto> sortDtosById(List<Dto> list) { list.sort(new Comparator<Dto>() { public int compare(Dto o1, Dto o2) { if (o1.getId() < o2.getId()) return -1; if (o1.getId() > o2.getId()) return 1; return 0; } }); return list; }
手首を軽く押すだけで
List<Dto> sortDtosById(List<Dto> list) { list.sort(Comparator.comparing(Dto::getId)); return list; }
ところで、「Ideas」の新しいバージョンでは、次のようにすることができます。
オプションの虐待
おそらく次のようなものを見たでしょう。
List<UserEntity> getUsersForGroup(Long groupId) { Optional<Long> optional = Optional.ofNullable(groupId); if (optional.isPresent()) { return userRepository.findUsersByGroup(optional.get()); } return Collections.emptyList(); }
多くの場合、 Optional
値の有無を確認するために使用されますが、このために作成されたものではありません。 虐待を結びつけて簡単に書く：
List<UserEntity> getUsersForGroup(Long groupId) { if (groupId == null) { return Collections.emptyList(); } return userRepository.findUsersByGroup(groupId); }
Optional
はメソッドの引数やフィールドに関するものではなく、戻り値に関するものであることを忘れないでください。 そのため、シリアル化サポートなしで設計されています。
引数の状態を変更するvoidメソッド
次のような方法を想像してください。
@Component @RequiredArgsConstructor public class ContractUpdater { private final ContractRepository repository; @Transactional public void updateContractById(Long contractId, Dto contractDto) { Contract contract = repository.findOne(contractId); contract.setValue(contractDto.getValue()); repository.save(contract); } }
確かにあなたは何度も似たようなものを見たり書いたりしました。 ここでは、メソッドがエンティティの状態を変更することを好みませんが、それを返しません。 同様のフレームワークメソッドはどのように動作しますか？ たとえば、 org.springframework.data.jpa.repository.JpaRepository::save
およびjavax.persistence.EntityManager::merge
は値を返します。 コントラクトを更新した後、 update
メソッドの外側で取得する必要があるとします。 次のようになります。
@Transactional public void anotherMethod(Long contractId, Dto contractDto) { updateService.updateContractById(contractId, contractDto); Contract contract = repositoroty.findOne(contractId); doSmth(contract); }
はい、エンティティを直接UpdateService::updateContract
に渡して、その署名を変更できますが、次のようにすることをおUpdateService::updateContract
。
@Component @RequiredArgsConstructor public class ContractUpdater { private final ContractRepository repository; @Transactional public Contract updateContractById(Long contractId, Dto contractDto) { Contract contract = repository.findOne(contractId); contract.setValue(contractDto.getValue()); return repository.save(contract); } } // @Transactional public void anotherMethod(Long contractId, Dto contractDto) { Contract contract = updateService.updateContractById(contractId, contractDto); doSmth(contract); }
これは、一方ではコードの簡素化に役立ち、他方ではテストに役立ちます。 一般に、 void
メソッドのテストは非常に退屈なタスクであり、同じ例を使用して示します。
@RunWith(MockitoJUnitRunner.class) public class ContractUpdaterTest { @Mock private ContractRepository repository; @InjectMocks private ContractUpdater updater; @Test public void updateContractById() { Dto dto = new Dto(); dto.setValue("- "); Long contractId = 1L; when(repository.findOne(contractId)).thenReturn(new Contract()); updater.updateContractById(contractId, contractDto); //void // , dto? - : ArgumentCaptor<Contract> captor = ArgumentCaptor.forClass(Contract.class); verify(repository).save(captor.capture()); Contract updated = captor.getValue(); assertEquals(dto.getValue(), updated.getValue()); } }
ただし、メソッドが値を返す場合、すべてをより簡単にすることができます。
@RunWith(MockitoJUnitRunner.class) public class ContractUpdaterTest { @Mock private ContractRepository repository; @InjectMocks private ContractUpdater updater; @Test public void updateContractById() { Dto dto = new Dto(); dto.setValue("- "); Long contractId = 1L; when(repository.findOne(contractId)).thenReturn(new Contract()); Contract updated = updater.updateContractById(contractId, contractDto); assertEquals(dto.getValue(), updated.getValue()); } }
一挙に、 ContractRepository::save
呼び出しだけでなく、保存された値の正確性もチェックされます。
自転車建設
楽しみのために、プロジェクトを開いてこれを探してください：
lastIndexOf('.')
高い確率で、式全体は次のようになります。
String fileExtension = fileName.subString(fileName.lastIndexOf('.'));
それは静的アナライザーが警告できないものであり、新しく発明された自転車に関するものです。 紳士、読み取り/書き込み/コピーのように、ファイルの名前/拡張子またはファイルへのパスに関連する特定の問題を解決している場合、10個のうち9個の場合、タスクはすでに解決済みです。 したがって、自転車の建設と結び付けて、既製の（そして実績のある）ソリューションを採用してください。
import org.apache.commons.io.FilenameUtils; //... String fileExtension = FilenameUtils.getExtension(fileName);
この場合、自転車の適合性の確認に費やす時間を節約し、より高度な機能を取得することもできます（ドキュメントFilenameUtils::getExtension
参照）。
または、1つのファイルの内容を別のファイルにコピーするコードは次のとおりです。
try { FileChannel sc = new FileInputStream(src).getChannel(); FileChannel dc = new FileOutputStream(new File(targetName)).getChannel(); sc.transferTo(0, sc.size(), dc); dc.close(); sc.close(); } catch (IOException ex) { log.error("", ex); }
どのような状況で私たちを防ぐことができますか？ 数千人：
- 宛先は、ファイルではなくフォルダである場合があります
- ソースはフォルダかもしれません
- ソースと宛先は同じファイルである可能性があります
- 宛先を作成できません
- などなど。
悲しいことは、それについて自己記録を使用して、コピー中にすでに学んだことです。
賢くやるなら
import org.apache.commons.io.FileUtils; try { FileUtils.copyFile(src, new File(targetName)); } catch (IOException ex) { log.error("", ex); }
次に、コピーの開始前にチェックの一部が実行され、可能性のある例外がより有益になります（ FileUtils::copyFile
ソースを参照）。
無視@ Nullable / @ NotNull
エンティティがあるとします：
@Entity @Getter public class UserEntity { @Id private Long id; @Column private String email; @Column private String petName; }
この場合、テーブルのemail
列は、petNameとは異なり、 not null
でないと記述されてnot null
。 つまり、対応する注釈でフィールドをマークできます。
import javax.annotation.Nullable; import javax.annotation.NotNull; //... @Column @NotNull private String email; @Column @Nullable private String petName;
一見、これは開発者にとってはヒントのように見えますが、実際はそうです。 同時に、これらの注釈は、通常のラベルよりもはるかに強力なツールです。
たとえば、開発環境はそれらを理解しており、注釈を追加した後、次のようにしようとします。
boolean checkIfPetBelongsToUser(UserEnity user, String lostPetName) { return user.getPetName().equals(lostPetName); }
アイデアはこのメッセージで危険を警告します：
メソッド呼び出し「equals」は「NullPointerException」を生成する場合があります
コード内
boolean hasEmail(UserEnity user) { boolean hasEmail = user.getEmail() == null; return hasEmail; }
別の警告があります：
条件「user.getEmail（）== null」は常に「false」です
これにより、組み込みの検閲者が考えられるエラーを検出し、コードの実行をよりよく理解できます。 同じ目的で、値とその引数を返すメソッドに注釈を付けると便利です。
私の議論が決定的でないように思える場合は、同じ「Spring」という深刻なプロジェクトのソースコードを見てください。それらはクリスマスツリーのような注釈でハングアップしています。 そして、これは気まぐれではなく、深刻な必要性です。
唯一の欠点は、私には、現代の状態でアノテーションを常に維持する必要があるようです。 ただし、見れば、それはむしろ祝福です。何度も何度もコードに戻ると、それがよりよく理解できるからです。
不注意
このコードにはエラーはありませんが、余分な部分があります。
Collection<Dto> dtos = getDtos(); Stream.of(1,2,3,4,5) .filter(id -> { List<Integer> ids = dtos.stream().map(Dto::getId).collect(toList()); return ids.contains(id); }) .collect(toList());
ストリームを通過するときに変更されない場合、検索が実行されるキーの新しいリストを作成する理由は明確ではありません。 要素が5つしかないのは良いことです。要素が100500個あるとしたら？ また、 getDtos
メソッドが100500個のオブジェクト（リストにある！）を返す場合、このコードのパフォーマンスはどうなりますか？ なし。したがって、次のようにした方が良いでしょう。
Collection<Dto> dtos = getDtos(); Set<Integer> ids = dtos.stream().map(Dto::getId).collect(toSet()); Stream.of(1,2,3,4,5) .filter(ids::contains) .collect(toList());
こちらも：
static <T, Q extends Query> void setParams( Map<String, Collection<T>> paramMap, Set<String> notReplacedParams, Q query) { notReplacedParams.stream() .filter(param -> paramMap.keySet().contains(param)) .forEach(param -> query.setParameter(param, paramMap.get(param))); }
明らかに、 inParameterMap.keySet()
式によって返される値は変更されていないため、変数に移動できます。
static <T, Q extends Query> void setParams( Map<String, Collection<T>> paramMap, Set<String> notReplacedParams, Q query) { Set<String> params = paramMap.keySet(); notReplacedParams.stream() .filter(params::contains) .forEach(param -> query.setParameter(param, paramMap.get(param))); }
ところで、このようなセクションは、「ループ内のオブジェクト割り当て」チェックを使用して計算できます。
静的解析が無力な場合
8番目のJavaは長い間死んでいますが、私たちは皆ストリームを愛しています。 私たちの何人かはそれらをとても愛しているので、どこでもそれらを使用します：
public Optional<EmailAdresses> getUserEmails() { Stream<UserEntity> users = getUsers().stream(); if (users.count() == 0) { return Optional.empty(); } return users.findAny(); }
ご存じのとおり、ストリームは終了が呼び出される前に新鮮であるため、コード内のusers
変数に再アクセスするとIllegalStateException
が発生します。
静的アナライザーは、そのようなエラーを報告する方法をまだ知らないため、タイムリーにキャッチする責任はレビューアーにあります。
Stream
型の変数を使用し、それらを引数として渡し、メソッドから戻ることは、地雷原を歩くようなものであるように思えます。 たぶん幸運かもしれませんが、そうではないかもしれません。 したがって、単純なルール：コード内のStream<T>
出現はすべてチェックする必要があります（ただし、すぐに削除することをお勧めします）。
単純型
boolean
やint
などは、パフォーマンスに関するものであると多くの人が確信しています。 これは部分的に当てはまりますが、さらに、デフォルトでは単純型はnot null
はありnot null
。 エンティティの整数フィールドが、テーブルでnot null
宣言されている列を参照する場合、 Integer
ではなくint
を使用するのが理にかなっています。 これは一種のコンボです。また、メモリ消費量が少なく、 null
不要なチェックによりコードが簡素化されnull
。
それだけです 上記のすべてが究極の真実ではないことを忘れないでください。自分の頭で考え、アドバイスの適用に有意義にアプローチしてください。