Czy rozbić klasę na kilka mniejszych? Ogólne pytania o clean code.

1

Dzień dobry,
Uczę się programowania i w ramach nauki praktycznie skończyłam pisanie symulatora mikroprocesora 6502. Mam kilka pytań odnośnie kodu.

  1. Klasa Cpu ma prawie 1k linii kodu czy to nie za dużo? Chciałam to jakoś rozbić ale nie wiem jak się za to zabrać praktycznie każdy tryb adresowania potrzebuje dostępu do pamięci jak również rejestrów procesora, więc musiałabym stworzyć nową klasę Registers przenieść tam rejestry i następnie przekazywać każdej z 13 metod zarówno Registers jak i Memory, ten sam problem pojawia się w przypadku operacji których jest 56. Z drugiej strony czy w ogóle powinno się rozbijać Cpu? Wydaje mi się, że tryby adresowania jak i operacje są integralną częścią Cpu ale trochę przeraża mnie klasa z 1000 linii kody i czy nie łamię SRP
  2. Czy zwracanie AddressModeResult, OperationResult jest dobrym pomysłem czy może powinnam utworzyć property public ushort AbsoluteAddress { get; private set; } i na przykład w metodzie AddressMode_ABS zamiast zwracać wynik ustawić pole AbsoluteAddress = address; i nic nie zwracać?
  3. Praktycznie w każdym trybie adresowania powtarzają mi się linie pobierające byte z pamięci i przekształcenie little endian na big endian czy to łamie zasadę DRY i czy powinnam wydzielić jakąś/jakieś metody do obsługi tego?
  4. Czy funkcje AddressMode_XXX i Operation_XXX powinny być prywatne a jako publiczne tylko zrobić np. Reset(), IRQ(), NMI(), Run() tylko jak wtedy testować prywatne metody czy powinno je się testować osobno?

Fragmenty programu

public class Cpu
{
	private readonly Instruction[] _instructions = new Instruction[256];
	private readonly Memory _memory;

    // registers
	public byte A { get; set; }
	public byte X { get; set; }
	public byte Y { get; set; }
	public byte SP { get; set; }
	public ushort PC { get; set; }

    // flags
	public bool C { get; set; }
	public bool Z { get; set; }
	public bool I { get; set; }
	public bool D { get; set; }
	public bool B { get; set; }
	public bool R { get; set; }
	public bool V { get; set; }
	public bool N { get; set; }

    // ...

    public void Run() { } // start executing
    public void IRQ() { } // interruption
    public void NMI() { } // interruption
    public void Reset() { } // reset cpu

    // ...

    // addressing modes (13)
    public AddressModeResult AddressMode_ABS()
    {
    	var lo = _memory.ReadByte(PC++);
    	var hi = _memory.ReadByte(PC++);
    	var address = (ushort)((hi << 8) | lo);

    	return new AddressModeResult(address);
    }

    public AddressModeResult AddressMode_ABX()
    {
    	var lo = _memory.ReadByte(PC++);
    	var hi = _memory.ReadByte(PC++);
    	var address = (ushort)(((hi << 8) | lo) + X);

    	return (address & 0xFF00) == hi << 8
    		? new AddressModeResult(address)
    		: new AddressModeResult(address, true);
    }

    public AddressModeResult AddressMode_ABY()
    {
    	var lo = _memory.ReadByte(PC++);
    	var hi = _memory.ReadByte(PC++);
    	var address = (ushort)(((hi << 8) | lo) + Y);

    	return (address & 0xFF00) == hi << 8
    		? new AddressModeResult(address)
    		: new AddressModeResult(address, true);
    }

    // ...

    // operations (56)
    public OperationResult Operation_LDA(ushort address)
    {
    	A = Fetch(address);
    	Z = A == 0x00;
    	N = (A & 0x80) != 0;

    	return new OperationResult(true, 1);
    }

    public OperationResult Operation_LDX(ushort address)
    {
    	X = Fetch(address);
    	Z = X == 0x00;
    	N = (X & 0x80) != 0;

    	return new OperationResult(true, 1);
    }

    public OperationResult Operation_LDY(ushort address)
    {
    	Y = Fetch(address);
    	Z = Y == 0x00;
    	N = (Y & 0x80) != 0;

    	return new OperationResult(true, 1);
    }
}

// analogicznie OperationResult
public class AddressModeResult
{
	public ushort Address { get; }
	public bool AdditionalCycle { get; }

	public AddressModeResult(ushort address = 0x0000, bool additionalCycle = false)
	{
		Address = address;
		AdditionalCycle = additionalCycle;
	}
}
  1. Podczas pisania posta nasunął mi się pomysł zrobienia czegoś takiego ale czy to nie będzie specjalne komplikowanie overengineering?
public interface IAddressMode
{
    AddressModeResult Calculate();
}

public class AbsoluteAddressMode : IAddressMode
{
    public AbsoluteAddressMode(Register register, Memory memory)
    { 
        // ...
    }

    public AddressModeResult Calculate()
    {
    	// ...

    	return new AddressModeResult();
     }
}

public interface IOperation
{
    OperationResult Run();
}

public class Operation_LDA : IOperation
{
    public Operation_LDA(Register register, Memory memory)
    {
        // ...
    }

    public OperationResult Run()
    {
        // ...

        return OperationResult();
    }
}

public class InstructionFactory
{
    public InstructionFactory(Registers registers, Memory memory)
    {
        // ...
    }

    public Instruction GetInstruction(byte opcode)
    {
        return opcode switch
        {
            0xA4 => new Instruction(new AddressModeZeroPage(_registers, _memory), new Operation_LDA(_register, _memory)),
            // ...
            _ => throw new InvalidOperationException();
        }
    }
}
public class Cpu
{
    private Memory _memory;
    private Registers _registers;
    private InstructionFactory _factory;

    public void Run()
    {
        var opcode = _memory.ReadByte(_registers.PC++);
        var instruction = _factory.getInstruction(opcode);

        instruction.Execute();
    }
}
1

tak

0

AD 1. Klasa może mieć 1k linii kodu. Jeżeli widzisz jak ją rozbić i da Ci to jaką wartość możesz to zrobić. Registers możesz spróbować wyizolować (enkapsulacja) żeby zamiast prymitywów A, X, Y, SP, PC mieć jeden obiekt zawierający wszystkie rejestry. Możesz wtedy mieć pole: private Registers registers i odwoływać się do niego z metod. Podobnie z flagami. Nie musisz przekazywać ich przez parametr, wystarczy że będziesz miała pole.

AD 2. To zależy... Takie rzeczy wychodzą w trakcie korzystania z klasy czyli kiedy zrobisz new Cpu() i będziesz korzystać z wystawionych operacji. Istnieje reguła YAGNI - "You aren't gonna need it", jeżeli ta klasa powstała z potrzeby - to wtedy możesz zobaczyć czy spełnia Twoją potrzebę. Jeżeli jest to "sztuczna" klasa którą stworzyłaś jako cel sam w sobie, to trudno zweryfikować czy jest dobrze zrobiona.

AD 3. Jeżeli możesz nie powtarzać kodu to zrób to, szczególnie jak możesz nazwać fragment lepszą nazwą.

// addressing modes (13)
public AddressModeResult AddressMode_ABS()
{
	var address = GetAddress(0);
	return new AddressModeResult(address);
}

public AddressModeResult AddressMode_ABX()
{
	var address = GetAddress(X);
	return Dunno(adress);
}


public AddressModeResult AddressMode_ABY()
{
    var address = GetAddress(Y);
    return Dunno(address);
}

private ushort GetAddress(byte value)
{
  var lo = _memory.ReadByte(PC++);
  var hi = _memory.ReadByte(PC++);
  var address = (ushort)((hi << 8) | lo + value);
  return address;
}

private AdressModeResult Dunno(ushort address) 
{
    return (address & 0xFF00) == hi << 8
    	? new AddressModeResult(address)
    	: new AddressModeResult(address, true);
}
public OperationResult Operation_LDX(ushort address)
{
	X = Fetch(address);
    return Operation_LD(X);
}

private OperationResult Operation_LD(byte value) 
{
    Z = value == 0x00;
    N = (value & 0x80) != 0;
    
    return new OperationResult(true, 1);
}

AD 4. To zależy od przypadków użycia. Jak nie używasz klasy CPU jako klient to się nie dowiesz jak to "należy" zrobić. Natomiast prywatnych metod lepiej nie testować, to są szczegóły implementacji. Testujesz widoczne rezultaty czy to przez Run() czy inne publiczne metody.
AD 5. Kwestia gustu.

1 użytkowników online, w tym zalogowanych: 0, gości: 1