Министерство образования, науки, молодежи и спорта Украины

Одесский национальный политехнический университет

Институт компьютерных систем

Кафедра информационных систем

Лабораторная работа № 3

По дисциплине: «Качество и надёжность»

На тему: «Проведение процедуры code review»

Выполнил:

ст. гр. АИ-091

Подкошин А.С.

Проверил:

Трофимов Б.Ф.

Одесса, 2013

Цель работы: необходимо выполнить code review для своего проекта из лабораторной работы 1(а-б). Кроме своего проекта необходимо выбрать уникальный в пределах потока Open Source проект из сайта Github на языке Java общим размером программного кода не менее 3000 строк кода (3000 loc). Для выбранного проекта провести процедуру code review.

Список критериев, по которым будет проводиться оценка качества кода:

* следование Java conventions

* Документированность публичных классов, публичных методов и интерфейсов, следование java docs

* Соблюдение Single Responsibility Principle

* Соблюдение Open Closed Principle

* Соблюдение Liskov Substitution Principle

* Соблюдение Dependency Inversion Principle

* Соблюдение Interface Segregation Principle

* Соблюдение принципа "Project Build Requires Only One Step"

* Соблюдение "Executing Tests Requires Only One Step"

* Предпочтение полиморфизму при использовании If/Else or Switch/Case

* Члены класса не хранят промежуточные данные (temporal state)

* Имена классов, методов и переменных отражают суть их предназначения

* Методы классов должны выполнять только одну задачу.

* Методы не содержать слишком большое количество входных параметров (больше 5)

* Код не содержит Magic Numbers / Strings

* Использование Exceptions вместо of Return Codes or null

* Каждый модульный тест удовлетворяет правилу AAA (arrange, act, assert)

* Каждый модульный тест изолирует класс от зависимостей.

* Names reflect what is tested, e.g. FeatureWhenScenarioThenBehaviour.

* Single Scenario per Test

* Нахождение теста в пакете совпадает с аналогичным в тестируемом классе.

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

Ход работы

1. Провёл процедуру code review (оценки качества программного кода) для проекта из лабораторной работы 1(а-б).

Нарушения:

Beginning Comments All source files should begin with a c-style comment that lists the class name, version information, date, and copyright notice.

/*

* @(#) PaymentsRepositoryImpl.java 1.82 99/03/18

*

* Copyright (c) 1994-1999 Sun Microsystems, Inc.

* 901 San Antonio Road, Palo Alto, California, 94303, U.S.A.

* All rights reserved.

*

* This software is the confidential and proprietary information of Sun

* Microsystems, Inc. ("Confidential Information"). You shall not

* disclose such Confidential Information and shall use it only in

* accordance with the terms of the license agreement you entered into

* with Sun.

*/

package laba2.domain;

import java.sql.Date;

import java.util.Iterator;

import java.util.List;

import org.hibernate.Criteria;

import org.hibernate.HibernateException;

import org.hibernate.SQLQuery;

import org.hibernate.Session;

import org.hibernate.SessionFactory;

import org.hibernate.Transaction;

import org.hibernate.cfg.Configuration;

import org.springframework.beans.factory.annotation.Autowired;

import org.springframework.stereotype.Repository;

import org.springframework.transaction.annotation.Transactional;

public class PaymentsRepositoryImpl implements PaymentsRepository{

. . . . .

}

Class/interface documentation comment (/**...*/)

package laba2.domain;

import java.sql.Date;

import java.util.Iterator;

import java.util.List;

import org.hibernate.Criteria;

import org.hibernate.HibernateException;

import org.hibernate.SQLQuery;

import org.hibernate.Session;

import org.hibernate.SessionFactory;

import org.hibernate.Transaction;

import org.hibernate.cfg.Configuration;

import org.springframework.beans.factory.annotation.Autowired;

import org.springframework.stereotype.Repository;

import org.springframework.transaction.annotation.Transactional;

/**

*

Class description goes here.

*

* @version

1.82 18 Mar 1999 * @author

Firstname Lastname

*/

public class PaymentsRepositoryImpl implements PaymentsRepository{

. . . . .

}

Wrapping Lines.When an expression will not fit on a single line, break it.

public Payment(String name, String surname, Date date, float amount, String currency) {

this.name = name;

this.surname = surname;

this.date = date;

this.amount = amount;

this.currency = currency;

}

if, if-else, if else-if else Statements

@Transactional

public List<Payment> getPaymentsByAmount(float amount) {

if (amount<0) throw new NegativeAmountException();

List<Payment> payments=repository.getAllPayments();

for (Iterator iterator =

payments.iterator(); iterator.hasNext();){

Payment pay = (Payment) iterator.next();

if (pay.getAmount()<amount) { iterator.remove(); }

Dead comments

public List<Payment> getAllPayments() {

Session session = factory.openSession();

session.beginTransaction();

List<Payment> payments = session.createQuery("FROM Payment").list();

for (Iterator iterator =

payments.iterator(); iterator.hasNext();){

Payment pay = (Payment) iterator.next();

/* System.out.print("First Name: " + pay.getName());

System.out.print(" Last Name: " + pay.getSurname());

System.out.println(" Salary: " + pay.getAmount());*/

}

session.close();

return(payments);

}

2. Выбрал Open Source проект библиотеки high-scale-lib на Github по адресу https://github.com/boundary/high-scale-lib.

3. Провёл code review проекта high-scale-lib и выявил следующие нарушения.

Нарушения:

Defining a method with one line of code

public void readOnly() {

throw new RuntimeException("Unimplemented");

}

Using magic numbers and strings

public class BitPrint {

public static String fmt(long bits) {

StringBuilder sb = new StringBuilder();

long mask = 1L<<63;

for(int i = 1; i <= 64; i++) {

if((mask & bits) == mask)

sb.append("1");

else

sb.append("0");

if(i%8 == 0)

sb.append("|");

mask >>>= 1;

}

return sb.toString();

}

Opacity. The code is hard to understand.

public class ConcurrentAutoTable implements Serializable {

// --- public interface ---

/**

* Add the given value to current counter value. Concurrent updates will

* not be lost, but addAndGet or getAndAdd are not implemented because the

* total counter value (i.e., {@link #get}) is not atomically updated.

* Updates are striped across an array of counters to avoid cache contention

* and has been tested with performance scaling linearly up to 768 CPUs.

*/

public void add( long x ) { add_if_mask( x,0); }

/** {@link #add} with -1 */

public void decrement() { add_if_mask(-1L,0); }

/** {@link #add} with +1 */

public void increment() { add_if_mask( 1L,0); }

/** Atomically set the sum of the striped counters to specified value.

* Rather more expensive than a simple store, in order to remain atomic.

*/

public void set( long x ) {

CAT newcat = new CAT(null,4,x);

// Spin until CAS works

while( !CAS_cat(_cat,newcat) );

}

/**

* Current value of the counter. Since other threads are updating furiously

* the value is only approximate, but it includes all counts made by the

* current thread. Requires a pass over the internally striped counters.

*/

public long get() { return _cat.sum(0); }

/** Same as {@link #get}, included for completeness. */

public int intValue() { return (int)_cat.sum(0); }

/** Same as {@link #get}, included for completeness. */

public long longValue() { return _cat.sum(0); }

/**

* A cheaper {@link #get}. Updated only once/millisecond, but as fast as a

* simple load instruction when not updating.

*/

public long estimate_get( ) { return _cat.estimate_sum(0); }

/**

* Return the counter's {@code long} value converted to a string.

*/

public String toString() { return _cat.toString(0); }

/**

* A more verbose print than {@link #toString}, showing internal structure.

* Useful for debugging.

*/

public void print() { _cat.print(); }

/**

* Return the internal counter striping factor. Useful for diagnosing

* performance problems.

*/

public int internal_size() { return _cat._t.length; }

// Only add 'x' to some slot in table, hinted at by 'hash', if bits under

// the mask are all zero. The sum can overflow or 'x' can contain bits in

// the mask. Value is CAS'd so no counts are lost. The CAS is retried until

// it succeeds or bits are found under the mask. Returned value is the old

// value - which WILL have zero under the mask on success and WILL NOT have

// zero under the mask for failure.

private long add_if_mask( long x, long mask ) { return _cat.add_if_mask(x,mask,hash(),this); }

// The underlying array of concurrently updated long counters

private volatile CAT _cat = new CAT(null,4/*Start Small, Think Big!*/,0L);

private static final AtomicReferenceFieldUpdater<ConcurrentAutoTable,CAT> _catUpdater =

AtomicReferenceFieldUpdater.newUpdater(ConcurrentAutoTable.class,CAT.class, "_cat");

private boolean CAS_cat( CAT oldcat, CAT newcat ) { return _catUpdater.compareAndSet(this,oldcat,newcat); }

// Hash spreader

private static final int hash() {

int h = System.identityHashCode(Thread.currentThread());

// You would think that System.identityHashCode on the current thread

// would be a good hash fcn, but actually on SunOS 5.8 it is pretty lousy

// in the low bits.

h ^= (h>>>20) ^ (h>>>12); // Bit spreader, borrowed from Doug Lea

h ^= (h>>> 7) ^ (h>>> 4);

return h<<2; // Pad out cache lines. The goal is to avoid cache-line contention

}

// --- CAT -----------------------------------------------------------------

private static class CAT implements Serializable {

// Unsafe crud: get a function which will CAS arrays

private static final Unsafe _unsafe = UtilUnsafe.getUnsafe();

private static final int _Lbase = _unsafe.arrayBaseOffset(long[].class);

private static final int _Lscale = _unsafe.arrayIndexScale(long[].class);

private static long rawIndex(long[] ary, int i) {

assert i >= 0 && i < ary.length;

return _Lbase + i * _Lscale;

}

private final static boolean CAS( long[] A, int idx, long old, long nnn ) {

return _unsafe.compareAndSwapLong( A, rawIndex(A,idx), old, nnn );

}