ronantakizawa/codereview-bench huggingface dataset:
Repository: faif/python-patterns
File: patterns/structural/3-tier.py (Python)

Code before:
"""
*TL;DR
Separates presentation, application processing, and data management functions.
"""

from typing import Dict, KeysView, Optional, Type, TypeVar, Union

T = TypeVar("T")


class Data:
    """ Data Store Class """

    products = {
        "milk": {"price": 1.50, "quantity": 10},
        "eggs": {"price": 0.20, "quantity": 100},
        "cheese": {"price": 2.00, "quantity": 10},
    }

    def __get__(
        self, obj: Optional[T], klas: Type[T]
    ) -> Dict[str, Dict[str, Dict[str, Union[int, float]]]]:

        print("(Fetching from Data Store)")
        return {"products": self.products}


class BusinessLogic:
    """ Business logic holding data store instances """

    data = Data()

    def product_list(self) -> KeysView[str]:
        return self.data["products"].keys()

    def product_information(
        self, product: str
    ) -> Optional[Dict[str, Union[int, float]]]:
        return self.data["products"].get(product, None)


class Ui:
    """ UI interaction class """

    def __init__(self) -> None:
        self.business_logic = BusinessLogic()

Review comment (suggestion):
This is an example where I think types add more noise than value. I prefer to keep the code as is, since `__get__` is a standard special function and anyone can look it up

Code after:
"""
*TL;DR
Separates presentation, application processing, and data management functions.
"""

from typing import Dict, KeysView, Optional, Type, TypeVar, Union


class Data:
    """ Data Store Class """

    products = {
        "milk": {"price": 1.50, "quantity": 10},
        "eggs": {"price": 0.20, "quantity": 100},
        "cheese": {"price": 2.00, "quantity": 10},
    }

    def __get__(self, obj, klas):

        print("(Fetching from Data Store)")
        return {"products": self.products}


class BusinessLogic:
    """ Business logic holding data store instances """

    data = Data()

    def product_list(self) -> KeysView[str]:
        return self.data["products"].keys()

    def product_information(
        self, product: str
    ) -> Optional[Dict[str, Union[int, float]]]:
        return self.data["products"].get(product, None)


class Ui:
    """ UI interaction class """

    def __init__(self) -> None:
        self.business_logic = BusinessLogic()

    def get_product_list(self) -> None:
        print("PRODUCT LIST:")
        for product in self.business_logic.product_list():
Repository: faif/python-patterns
File: patterns/structural/3-tier.py (Python)

Code before:
        "eggs": {"price": 0.20, "quantity": 100},
        "cheese": {"price": 2.00, "quantity": 10},
    }

    def __get__(
        self, obj: Optional[T], klas: Type[T]
    ) -> Dict[str, Dict[str, Dict[str, Union[int, float]]]]:

        print("(Fetching from Data Store)")
        return {"products": self.products}


class BusinessLogic:
    """ Business logic holding data store instances """

    data = Data()

    def product_list(self) -> KeysView[str]:
        return self.data["products"].keys()

    def product_information(
        self, product: str
    ) -> Optional[Dict[str, Union[int, float]]]:
        return self.data["products"].get(product, None)


class Ui:
    """ UI interaction class """

    def __init__(self) -> None:
        self.business_logic = BusinessLogic()

    def get_product_list(self) -> None:
        print("PRODUCT LIST:")
        for product in self.business_logic.product_list():
            print(product)
        print("")

    def get_product_information(self, product: str) -> None:
        product_info = self.business_logic.product_information(product)
        if product_info:
            print("PRODUCT INFORMATION:")
            print(
                f"Name: {product.title()}, "
                + f"Price: {product_info.get('price', 0):.2f}, "
                + f"Quantity: {product_info.get('quantity', 0):}"
            )
        else:
            print(f"That product '{product}' does not exist in the records")



Review comment (suggestion):
Same here. `None` is the default return value so it makes sense for an `__init__` function. I don't find it helpful 

Code after:
    }

    def __get__(self, obj, klas):

        print("(Fetching from Data Store)")
        return {"products": self.products}


class BusinessLogic:
    """ Business logic holding data store instances """

    data = Data()

    def product_list(self) -> KeysView[str]:
        return self.data["products"].keys()

    def product_information(
        self, product: str
    ) -> Optional[Dict[str, Union[int, float]]]:
        return self.data["products"].get(product, None)


class Ui:
    """ UI interaction class """

    def __init__(self) -> None:
        self.business_logic = BusinessLogic()

    def get_product_list(self) -> None:
        print("PRODUCT LIST:")
        for product in self.business_logic.product_list():
            print(product)
        print("")

    def get_product_information(self, product: str) -> None:
        product_info = self.business_logic.product_information(product)
        if product_info:
            print("PRODUCT INFORMATION:")
            print(
                f"Name: {product.title()}, "
                + f"Price: {product_info.get('price', 0):.2f}, "
                + f"Quantity: {product_info.get('quantity', 0):}"
            )
        else:
            print(f"That product '{product}' does not exist in the records")


def main():
    ui = Ui()
    ui.get_product_list()
    ui.get_product_information("cheese")
Repository: faif/python-patterns
File: patterns/structural/3-tier.py (Python)

Code before:
    def __get__(self, obj, klas):
        print("(Fetching from Data Store)")
        return {'products': self.products}


class BusinessLogic:
    """ Business logic holding data store instances """

    data = Data()

    def product_list(self):
        return self.data['products'].keys()

    def product_information(self, product):
        return self.data['products'].get(product, None)


class Ui:
    """ UI interaction class """

    def __init__(self):
        self.business_logic = BusinessLogic()

    def get_product_list(self):
        print('PRODUCT LIST:')
        for product in self.business_logic.product_list():
            print(product)
        print('')

    def get_product_information(self, product):
        product_info = self.business_logic.product_information(product)
        if product_info:
            print('PRODUCT INFORMATION:')
            print(
                'Name: {0}, Price: {1:.2f}, Quantity: {2:}'.format(
                    product.title(), product_info.get('price', 0), product_info.get('quantity', 0)
                )
            )
        else:
            print('That product "{0}" does not exist in the records'.format(product))


def main():
    ui = Ui()
    ui.get_product_list()
    ui.get_product_information('cheese')
    ui.get_product_information('eggs')
    ui.get_product_information('milk')
    ui.get_product_information('arepas')



Review comment (suggestion):
My only objection is with the `None` returns. Can you give me examples of other projects that use it? I don't see an added value.

Code after:
    }

    def __get__(self, obj, klas):

        print("(Fetching from Data Store)")
        return {"products": self.products}


class BusinessLogic:
    """ Business logic holding data store instances """

    data = Data()

    def product_list(self) -> KeysView[str]:
        return self.data["products"].keys()

    def product_information(
        self, product: str
    ) -> Optional[Dict[str, Union[int, float]]]:
        return self.data["products"].get(product, None)


class Ui:
    """ UI interaction class """

    def __init__(self) -> None:
        self.business_logic = BusinessLogic()

    def get_product_list(self) -> None:
        print("PRODUCT LIST:")
        for product in self.business_logic.product_list():
            print(product)
        print("")

    def get_product_information(self, product: str) -> None:
        product_info = self.business_logic.product_information(product)
        if product_info:
            print("PRODUCT INFORMATION:")
            print(
                f"Name: {product.title()}, "
                + f"Price: {product_info.get('price', 0):.2f}, "
                + f"Quantity: {product_info.get('quantity', 0):}"
            )
        else:
            print(f"That product '{product}' does not exist in the records")


def main():
    ui = Ui()
    ui.get_product_list()
    ui.get_product_information("cheese")
Repository: faif/python-patterns
File: patterns/creational/prototype.py (Python)

Code before:
*What does this example do?
When the number of prototypes in an application can vary, it can be
useful to keep a Dispatcher (aka, Registry or Manager). This allows
clients to query the Dispatcher for a prototype before cloning a new
instance.

Below provides an example of such Dispatcher, which contains three
copies of the prototype: 'default', 'objecta' and 'objectb'.

*TL;DR
Creates new object instances by cloning prototype.
"""


class Prototype:

    value = "default"

    def clone(self, **attrs):
        """Clone a prototype and update inner attributes dictionary"""
        # Python in Practice, Mark Summerfield
        obj = self.__class__()
        obj.__dict__.update(attrs)
        return obj


class PrototypeDispatcher:
    def __init__(self):
        self._objects = {}

    def get_objects(self):
        """Get all objects"""
        return self._objects

    def register_object(self, name, obj):
        """Register an object"""
        self._objects[name] = obj

    def unregister_object(self, name):
        """Unregister an object"""
        del self._objects[name]


def main():
    """
    >>> dispatcher = PrototypeDispatcher()
    >>> prototype = Prototype()

    >>> d = prototype.clone()
    >>> a = prototype.clone(value='a-value', category='a')
    >>> b = prototype.clone(value='b-value', is_checked=True)

Review comment (suggestion):
It is possible like below if this prototype is just copy the `value` into clones.
```
obj = self.__class__(value=self.value)
```

Code after:
*What does this example do?
When the number of prototypes in an application can vary, it can be
useful to keep a Dispatcher (aka, Registry or Manager). This allows
clients to query the Dispatcher for a prototype before cloning a new
instance.

Below provides an example of such Dispatcher, which contains three
copies of the prototype: 'default', 'objecta' and 'objectb'.

*TL;DR
Creates new object instances by cloning prototype.
"""

from typing import Any, Dict


class Prototype:
    def __init__(self, value: str = "default", **attrs: Any) -> None:
        self.value = value
        self.__dict__.update(attrs)

    def clone(self, **attrs: Any) -> None:
        """Clone a prototype and update inner attributes dictionary"""
        # Python in Practice, Mark Summerfield
        # copy.deepcopy can be used instead of next line.
        obj = self.__class__(**self.__dict__)
        obj.__dict__.update(attrs)
        return obj


class PrototypeDispatcher:
    def __init__(self):
        self._objects = {}

    def get_objects(self) -> Dict[str, Prototype]:
        """Get all objects"""
        return self._objects

    def register_object(self, name: str, obj: Prototype) -> None:
        """Register an object"""
        self._objects[name] = obj

    def unregister_object(self, name: str) -> None:
        """Unregister an object"""
        del self._objects[name]


def main() -> None:
    """
    >>> dispatcher = PrototypeDispatcher()
    >>> prototype = Prototype()
Repository: fastfetch-cli/fastfetch
File: src/common/settings.c (C)

Code before:

    data.ffsqlite3_open_v2 = FF_LIBRARY_LOAD_SYMBOL(library, "sqlite3_open_v2", mutex, 0);
    data.ffsqlite3_prepare_v2 = FF_LIBRARY_LOAD_SYMBOL(library, "sqlite3_prepare_v2", mutex, 0);
    data.ffsqlite3_step = FF_LIBRARY_LOAD_SYMBOL(library, "sqlite3_step", mutex, 0);
    data.ffsqlite3_data_count = FF_LIBRARY_LOAD_SYMBOL(library, "sqlite3_data_count", mutex, 0);
    data.ffsqlite3_column_int = FF_LIBRARY_LOAD_SYMBOL(library, "sqlite3_column_int", mutex, 0);
    data.ffsqlite3_finalize = FF_LIBRARY_LOAD_SYMBOL(library, "sqlite3_finalize", mutex, 0);
    data.ffsqlite3_close = FF_LIBRARY_LOAD_SYMBOL(library, "sqlite3_close", mutex, 0);

    pthread_mutex_unlock(&mutex);
    return getSQLiteColumnCount(&data, fileName, tableName);
}
#else //FF_HAVE_SQLITE3
uint32_t ffSettingsGetSQLiteColumnCount(FFinstance* instance, const char* fileName, const char* tableName)
{
    FF_UNUSED(instance, fileName, tableName);
    return 0;
}
#endif //FF_HAVE_SQLITE3

#ifdef __ANDROID__
#if __has_include(<sys/system_properties.h>)
#include <sys/system_properties.h>
void ffSettingsGetAndroidProperty(const char* propName, FFstrbuf* result) {
    char str[PROP_VALUE_MAX + 1];
    if (__system_property_get(propName, str)) {
        ffStrbufSetS(result, str);
    }
}
#else
#include <cutils/properties.h>
void ffSettingsGetAndroidProperty(const char* propName, FFstrbuf* result) {
    char str[PROP_VALUE_MAX + 1] = "0";
    property_get(propName, str, "0");
    ffStrbufSetS(result, str);
}
#endif
#endif


Review comment (suggestion):
This should be a bit more performant:
```c
ffStrbufEnsureCapacity(result, PROP_VALUE_MAX);
result->length = __system_property_get(propName, result->chars);
```

Code after:

    data.ffsqlite3_open_v2 = FF_LIBRARY_LOAD_SYMBOL(library, "sqlite3_open_v2", mutex, 0);
    data.ffsqlite3_prepare_v2 = FF_LIBRARY_LOAD_SYMBOL(library, "sqlite3_prepare_v2", mutex, 0);
    data.ffsqlite3_step = FF_LIBRARY_LOAD_SYMBOL(library, "sqlite3_step", mutex, 0);
    data.ffsqlite3_data_count = FF_LIBRARY_LOAD_SYMBOL(library, "sqlite3_data_count", mutex, 0);
    data.ffsqlite3_column_int = FF_LIBRARY_LOAD_SYMBOL(library, "sqlite3_column_int", mutex, 0);
    data.ffsqlite3_finalize = FF_LIBRARY_LOAD_SYMBOL(library, "sqlite3_finalize", mutex, 0);
    data.ffsqlite3_close = FF_LIBRARY_LOAD_SYMBOL(library, "sqlite3_close", mutex, 0);

    pthread_mutex_unlock(&mutex);
    return getSQLiteColumnCount(&data, fileName, tableName);
}
#else //FF_HAVE_SQLITE3
uint32_t ffSettingsGetSQLiteColumnCount(FFinstance* instance, const char* fileName, const char* tableName)
{
    FF_UNUSED(instance, fileName, tableName);
    return 0;
}
#endif //FF_HAVE_SQLITE3

#ifdef __ANDROID__
#include <sys/system_properties.h>
void ffSettingsGetAndroidProperty(const char* propName, FFstrbuf* result) {
    ffStrbufEnsureCapacity(result, PROP_VALUE_MAX);
    result->length = __system_property_get(propName, result->chars);
}
#endif
Repository: fastfetch-cli/fastfetch
File: packaging/deb/create-deb-pkg.sh (Shell)

Code before:
#!/bin/sh

cd "$(dirname "$0")"
echo "cleanup"
rm -rf ./build ./fastfetch fastfetch.deb

echo "building fastfetch"
mkdir build
cd build
cmake ../../..
cmake --build . -j$(nproc)

echo "generating package contents"
# deb versions must start with a number, hence the 1 added before the rXXX
version="0:1$(./fastfetch --version | cut -d ' ' -f2)-0"
echo "Using package version: $version"
cd ..

mkdir -p fastfetch/DEBIAN
sed "s/<VERSION>/$version/g" control-template > fastfetch/DEBIAN/control

mkdir -p fastfetch/usr/bin
cp build/fastfetch fastfetch/usr/bin/
cp build/flashfetch fastfetch/usr/bin/
mkdir -p fastfetch/usr/share/bash-completion/completions
cp ../../completions/bash fastfetch/usr/share/bash-completion/completions/fastfetch

echo "building package"
dpkg-deb --build fastfetch
echo "package built."
echo "cleaning up"
rm -rf build fastfetch


Review comment (suggestion):
`cmake --build . --target fastfetch`. -j should be configured by the user with the MAKEFLAGS environment variable, not forced by default.

Code after:
#!/bin/sh

cd "$(dirname "$0")" || exit
echo "cleanup"
rm -rf ./build ./fastfetch fastfetch.deb

echo "building fastfetch"
mkdir build
cd build || exit
cmake ../../..
cmake --build . --target fastfetch "-j$(nproc)"

echo "generating package contents"
# deb versions must start with a number, hence the 1 added before the rXXX
version="0:1$(./fastfetch --version | cut -d ' ' -f2)-0"
echo "Using package version: $version"
cd ..

mkdir -p fastfetch/DEBIAN
sed "s/<VERSION>/$version/g" control-template > fastfetch/DEBIAN/control

mkdir -p fastfetch/usr/bin
cp build/fastfetch fastfetch/usr/bin/
mkdir -p fastfetch/usr/share/bash-completion/completions
cp ../../completions/bash fastfetch/usr/share/bash-completion/completions/fastfetch
mkdir -p fastfetch/usr/share/fastfetch/
cp -r ../../presets fastfetch/usr/share/fastfetch/

echo "building package"
dpkg-deb --build fastfetch
echo "package built."
echo "cleaning up"
rm -rf build fastfetch
Repository: fastfetch-cli/fastfetch
File: src/fastfetch.c (C)

Code before:
    FFdata data;
    ffValuestoreInit(&data.customValues, sizeof(CustomValue));
    ffStrbufInitA(&data.structure, 256);
    data.loadUserConfig = true;

    parseConfigFileSystem(&instance, &data);
    parseConfigFileUser(&instance, &data);
    parseArguments(&instance, &data, argc, argv);

    //If we don't have a custom structure, use the default one
    if(data.structure.length == 0)
        ffStrbufAppendS(&data.structure, FASTFETCH_DATATEXT_STRUCTURE);

    if(ffStrbufContainIgnCaseS(&data.structure, ":cpuusage:"))
        ffPrepareCPUUsage();
    if(ffStrbufContainIgnCaseS(&data.structure, ":publicip:"))
        ffPreparePublicIp(&instance);
    if(ffStrbufContainIgnCaseS(&data.structure, ":weather:"))
        ffPrepareWeather(&instance);

    ffStart(&instance);

    //Parse the structure and call the modules
    uint32_t startIndex = 0;
    while (startIndex < data.structure.length)
    {
        uint32_t colonIndex = ffStrbufNextIndexC(&data.structure, startIndex, ':');
        data.structure.chars[colonIndex] = '\0';

        parseStructureCommand(&instance, &data, data.structure.chars + startIndex);

        startIndex = colonIndex + 1;
    }

    ffFinish(&instance);

    ffStrbufDestroy(&data.structure);
    ffValuestoreDestroy(&data.customValues);

    ffDestroyInstance(&instance);
}


Review comment (suggestion):
It should work without the `:` too, this way modules at the start and the end of the structure can be matched too.

Code after:
    FFdata data;
    ffValuestoreInit(&data.customValues, sizeof(CustomValue));
    ffStrbufInitA(&data.structure, 256);
    data.loadUserConfig = true;

    parseConfigFileSystem(&instance, &data);
    parseConfigFileUser(&instance, &data);
    parseArguments(&instance, &data, argc, argv);

    //If we don't have a custom structure, use the default one
    if(data.structure.length == 0)
        ffStrbufAppendS(&data.structure, FASTFETCH_DATATEXT_STRUCTURE);

    #define FF_CONTAINS_MODULE_NAME(moduleName)\
        ffStrbufContainIgnCaseS(&data.structure, ":" #moduleName ":") ||\
        ffStrbufStartsWithIgnCaseS(&data.structure, #moduleName ":") ||\
        ffStrbufEndsWithIgnCaseS(&data.structure, ":" #moduleName)

    if(FF_CONTAINS_MODULE_NAME(CPUUsage))
        ffPrepareCPUUsage();
    if(FF_CONTAINS_MODULE_NAME(PublicIp))
        ffPreparePublicIp(&instance);
    if(FF_CONTAINS_MODULE_NAME(Weather))
        ffPrepareWeather(&instance);
    #undef FF_CONTAINS_MODULE_NAME

    ffStart(&instance);

    //Parse the structure and call the modules
    uint32_t startIndex = 0;
    while (startIndex < data.structure.length)
    {
        uint32_t colonIndex = ffStrbufNextIndexC(&data.structure, startIndex, ':');
        data.structure.chars[colonIndex] = '\0';

        parseStructureCommand(&instance, &data, data.structure.chars + startIndex);

        startIndex = colonIndex + 1;
    }

    ffFinish(&instance);

    ffStrbufDestroy(&data.structure);
    ffValuestoreDestroy(&data.customValues);

    ffDestroyInstance(&instance);
}
Repository: fastfetch-cli/fastfetch
File: src/detection/terminalfont/terminalfont.c (C)

Code before:
        return false;

    if(fontName.length == 0)
        ffStrbufSetS(&fontName, "monospace");
    if(fontSize.length == 0)
        ffStrbufSetS(&fontSize, "11.0");

    ffFontInitValues(&result->font, fontName.chars, fontSize.chars);

    ffStrbufDestroy(&fontName);
    ffStrbufDestroy(&fontSize);

    return true;
}

static bool detectTerminator(const FFinstance* instance, FFTerminalFontResult* result)
{
    FFstrbuf fontName;
    ffStrbufInit(&fontName);

    FFpropquery fontQuery[] = {
        {"font =", &fontName},
    };

    if(!ffParsePropFileConfigValues(instance, "terminator/config", 1, fontQuery))
        return false;

    if(fontName.length == 0)
        ffStrbufSetS(&fontName, "Mono 8");

    ffFontInitCopy(&result->font, fontName.chars);

    ffStrbufDestroy(&fontName);

    return true;
}

static bool detectWezterm(FF_MAYBE_UNUSED const FFinstance* instance, FFTerminalFontResult* result)
{
    FF_STRBUF_AUTO_DESTROY fontName;
    ffStrbufInit(&fontName);

    ffStrbufSetS(&result->error, ffProcessAppendStdOut(&fontName, (char* const[]){
        "wezterm",
        "ls-fonts",
        "--text",
        "a",
        NULL
    }));
    if(result->error.length)
        return false;

Review comment (suggestion):
this can be simplified to:
```c
if(!ffParsePropFileConfig(instance, "terminator/config", "font =" &fontName))
    return false;
```

Code after:
        return false;

    if(fontName.length == 0)
        ffStrbufSetS(&fontName, "monospace");
    if(fontSize.length == 0)
        ffStrbufSetS(&fontSize, "11.0");

    ffFontInitValues(&result->font, fontName.chars, fontSize.chars);

    ffStrbufDestroy(&fontName);
    ffStrbufDestroy(&fontSize);

    return true;
}

static bool detectTerminator(const FFinstance* instance, FFTerminalFontResult* result)
{
    FFstrbuf fontName;
    ffStrbufInit(&fontName);

    if(!ffParsePropFileConfig(instance, "terminator/config", "font =", &fontName))
        return false;

    if(fontName.length == 0)
        ffStrbufSetS(&fontName, "Mono 8");

    ffFontInitCopy(&result->font, fontName.chars);

    ffStrbufDestroy(&fontName);

    return true;
}

static bool detectWezterm(FF_MAYBE_UNUSED const FFinstance* instance, FFTerminalFontResult* result)
{
    FF_STRBUF_AUTO_DESTROY fontName;
    ffStrbufInit(&fontName);

    ffStrbufSetS(&result->error, ffProcessAppendStdOut(&fontName, (char* const[]){
        "wezterm",
        "ls-fonts",
        "--text",
        "a",
        NULL
    }));
    if(result->error.length)
        return false;

    //LeftToRight
    // 0 a    \u{61}       x_adv=7  cells=1  glyph=a,180  wezterm.font("JetBrains Mono", {weight="Regular", stretch="Normal", style="Normal"})
    //                                      <built-in>, BuiltIn