Factory Method implementation Can you please verify my approach?

using System;

    /*
    * In Factory pattern, we create object without exposing the creation logic.
    * In this pattern, an interface is used for creating an object,
    * but let subclass decide which class to instantiate.
    * */

namespace FactoryMethod
{

Product (abstract)

    /*
    * Faza Class ITree
    *
    * */
    public interface ITree
    {
        string GetTreeName();
    }

Products (concrete)

    /*
    * The Concrete class which implements ITree
    *
    * */

    public class BananaTree : ITree
    {
        public string GetTreeName()
        {
            return "My Name Is Banana Tree";
        }
    }

    /*
    * The Concrete class which implements ITree
    *
    * */
    public class CoconutTree : ITree
    {
        public string GetTreeName()
        {
            return "My Name Is Coconut Tree";
        }
    }

Factory (abstract)

    /*
    * Faza Class TreeType
    * If you want you can add abstract class instad of faza class
    *
    * */
    public interface TreeType
    {
        ITree GetTree(string tree);
    }

Factory (concrete)

    /*
    * Concrete class which implements faza or concrete class
    *
    * */
    public class ConcreteTreeType : TreeType
    {

        public ITree GetTree(string tree)
        {

            if (tree == "COCONUT")
                return new CoconutTree();
            else
                return new BananaTree();

        }
    }

Client code

    /*
    * main app.
    *
    *  */
    class Program
    {
        static void Main(string[] args)
        {
            TreeType oTreeType = new ConcreteTreeType();

            ITree banana = oTreeType.GetTree("COCONUT");
            Console.WriteLine(banana.GetTreeName());

            Console.ReadKey();
        }
    }
}

I personally would not rely on using magic strings.

  Magic strings are where you have taken something like a class/method/variable name and written it within a string, which is then used to identify the appropriate class/method/variable

This makes it hard to refactor later on if you change a class name, it is too easy to miss instances etc. Furthermore, the code that you currently have is case-sensitive. This might be by design, however, consider the following:

var actuallyABananaTree = oTreeType.GetTree("Coconut");

Even though the developer is specifying a coconut tree, he actually gets a banana tree. If the construction logic was changed to:

if (tree.Equals("coconut", StringComparison.OrdinalIgnoreCase))

You would then get the right tree type back. See StringComparison Enum for the comparison options.

In that same construction logic, you are also not checking to see whether or not the parameter tree is a null reference. This might be something that you wish to consider adding, especially if you use the approach suggested above.

Use of Enums

A better approach would be to use enums instead of magic strings. For example:

enum MyTreeTypes {
      Coconut,
      Banana
}

Then in your construction logic, you can have:

public ITree GetTree(MyTreeTypes tree)
{
    switch (tree)
    {
        case MyTreeTypes.Coconut:
            return new CoconutTree();
        default:
            return new BananaTree();
    }
}

Using this approach ensures type safety and prevents problems of magic strings when refactoring etc. Also, you will compile time errors if you spell the tree type incorrectly


PHP PDO Database Wrapper I'm trying to wrap my head around OOP, and am fairly new to PHP in general, so please forgive me if the following makes you cringe.

I've decided to try and create a basic database wrapper using PDO. Here's what I've done so far.

class DB {
    protected $db_user = 'root';
    protected $db_pass = '';
    protected $db_host = 'localhost';
    protected $db_name = 'login';

    private $dbh;
    private $sth;

    public function __construct() {
        $this->dbh = new PDO('mysql:host='.$this->db_host.'; dbname='.$this->db_name, $this->db_user, $this->db_pass);
    }

    public function select($get, $table, $where){
        $this->sth = $this->dbh->prepare('SELECT '.$get.' FROM '.$table.' WHERE '.$where);
    }

    public function bindSTR($name, $replacement) {
        $this->sth->bindParam($name, $replacement, PDO::PARAM_STR);
    }

    public function execute(){
        $this->sth->execute();
    }

    public function fetch(){
        return $this->sth->fetch(PDO::FETCH_ASSOC);
    }
}

And here is the usage:

$DB = new DB();
$DB->select('user_id, email', 'users', 'email = :email');
$DB->bindSTR(':email', 'example@domain.com');
$DB->execute();
$results = $DB->fetch();

Now before I go ahead and start adding more functionality to the class, can anyone give me some feedback?

Is this a good way to go?
How can I improve the class?
Any suggestions in general?

Is this a good way to go? How can I improve the class? Any suggestions in general?

Right now, your class doesn't really add any functionality to PDO. It basically just renames it's methods (to names other programmers will now have to get familiar with), and reduces it's flexibility and functionality.

The only advantage you gain currently is that you save typing SELECT, FROM, and WHERE. I don't think that this is worth it.

Misc

use spaces around . for increased readability.
sth is a pretty common name, so it isn't that bad, but something like selectStatement would be a lot clearer. And what does dbh stand for?
I would probably rename get to columns (and I would probably make it an array so it's easier to use).


OpenGL Shader Loader in C++ I made my own custom OpenGL shader loader for C++. Please tell me is I'm doing anything incorrectly or anything to improve on. I've tested this and it works perfectly fine.
main.cc
std::vector<ShaderInfo> shader_info {
    {GL_VERTEX_SHADER, "shader.vert"},
    {GL_FRAGMENT_SHADER, "shader.frag"}
};
GLuint shader_program {LoadShaders(shader_info)};
glUseProgram(shader_program);

shader-loader.hh
struct ShaderInfo {
    GLenum type;
    std::string file;
};

GLuint LoadShaders(const std::vector<ShaderInfo>&);

shader-loader.cc
GLuint LoadShaders(const std::vector<ShaderInfo>& shader_info) {
    GLuint program {glCreateProgram()};
    std::vector<GLuint> detach;

    for (auto info : shader_info) {
        std::ifstream file {info.file, std::ios_base::in};
        file.seekg(0, std::ios_base::end);
        std::string source(file.tellg(), '\0');

        file.seekg(0, std::ios_base::beg);
        file.read(&source[0], source.size());
        file.close();

        GLuint shader {glCreateShader(info.type)};
        GLchar* source_ptr {&source[0]};
        glShaderSource(shader, 1, &source_ptr, nullptr);

        glCompileShader(shader);
        glAttachShader(program, shader);
        glDeleteShader(shader);
        detach.push_back(shader);

        GLint status;
        glGetShaderiv(shader, GL_COMPILE_STATUS, &status);

        if (status == GL_FALSE) {
            GLint length;
            glGetShaderiv(shader, GL_INFO_LOG_LENGTH, &length);

            std::string log(length, '\0');
            glGetShaderInfoLog(shader, length, nullptr, &log[0]);

            glDeleteProgram(program);
            throw std::runtime_error(log);
        }
    }

    glLinkProgram(program);
    for (auto shader : detach)
        glDetachShader(program, shader);

    GLint status;
    glGetProgramiv(program, GL_LINK_STATUS, &status);

    if (status == GL_FALSE) {
        GLint length;
        glGetProgramiv(program, GL_INFO_LOG_LENGTH, &length);

        std::string log(length, '\0');
        glGetProgramInfoLog(program, length, nullptr, &log[0]);

        glDeleteProgram(program);
        throw std::runtime_error(log);
    }

    return program;
}
`````

for (auto info : shader_info) -> for (auto const& info : shader_info) to avoid a string copy.

Reading the source file into a string should probably be a separate function.

We should throw an error if the file fails to open. (tellg will return -1 in that case).

I find it's often helpful to output more than one shader object log before stopping compilation (for example, if we have a minor error in the vertex shader, and another one in the fragment shader, we can see and fix both at once, rather than only having the vertex shader error in the log).

e.g.:
GLuint LoadShaders(const std::vector<ShaderInfo>& shader_info) {
    // ...
    bool compiled = true; // flag to indicate overall compile status

    for (auto const& info : shader_info) {
        // ... read file and compile shader

        GLint status;
        glGetShaderiv(shader, GL_COMPILE_STATUS, &status);

        if (status == GL_FALSE) {
            // ... get log string

            // log the error (if you have an existing logging system, or print to std::cerr)
            LogError("glCompileShader() failed for shader object: " + info.file);
            LogError(log);

            compiled = false; // set the overall flag to false
        }
    }

    // and now we throw!
    if (!compiled) {
        glDeleteProgram(program);
        throw std::runtime_error("Shader compilation failed!");
    }

    // ... link program etc. as before
}


remove_prefix and remove_suffix functions If the prefix/suffix does not match the beginning or the end of a string, then, depending on how I call these functions, they should either raise an exception or return the original text unmodified.

I am using these inside of a simple script for now, so there was no need to make these member functions of some class. I am hoping to find a way to simplify the logic, extract any common code into private function(s), improve the code style (although I think there are several different standards).

def remove_prefix(text, prefix, raise_if_no_match=True):
    exc_msg = 'Text "{}" does not start with a prefix "{}".'.format(text, prefix)
    if not prefix or not text:
        if not raise_if_no_match:
            return text
        if len(prefix) == len(text):
            return ''
        raise ValueError(exc_msg)
    if text.startswith(prefix):
        return text[len(prefix):]
    if raise_if_no_match:
        raise ValueError(exc_msg)
    return text

def remove_suffix(text, suffix, raise_if_no_match=True):
    exc_msg = 'Text "{}" does not end with a suffix "{}".'.format(text, suffix)
    if not suffix or not text:
        if not raise_if_no_match:
            return text
        if len(suffix) == len(text):
            return ''
        raise ValueError(exc_msg)
    if text.endswith(suffix):
        return text[:-len(suffix):]
    if raise_if_no_match:
        raise ValueError(exc_msg)
    return text

print remove_prefix('Hello, World', 'Hello, ')
# ValueError: Text "Hello, World" does not start with a prefix "Hello, Hello".
#print remove_prefix('Hello, World', 'Hello, Hello')
print remove_prefix('Hello, World', 'Hello, Hello', raise_if_no_match=False)

print remove_suffix('I am singing in the rain', ' in the rain')
# ValueError: Text "I am singing in the rain" does not end with a suffix "swinging in the rain".
#print remove_suffix('I am singing in the rain', 'swinging in the rain')
print remove_suffix('I am singing in the rain', 'swinging in the rain', raise_if_no_match=False)

Output:

World
Hello, World
I am singing
I am singing in the rain

Firstly I would say turn your print statements into a test. That way you can change the implementation with confidence that you have not broken anything.

Here is what those debug prints are as a test:

class TestRemove(unittest.TestCase):

    def test_remove_prefix(self):
        hello = 'Hello, World'
        value = remove_prefix(hello, 'Hello, ')
        self.assertEqual(value, 'World')
        value = remove_prefix(hello, 'Hello, Hello', raise_if_no_match=False)
        self.assertEqual(value, hello)
        self.assertRaises(ValueError, remove_prefix, hello, 'Hello, Hello')

    def test_remove_suffix(self):
        singing = 'I am singing in the rain'
        value = remove_suffix(singing, ' in the rain')
        self.assertEqual(value, 'I am singing')
        value = remove_suffix(singing, 'swinging in the rain', raise_if_no_match=False)
        self.assertEqual(value, singing)
        self.assertRaises(ValueError, remove_suffix, singing, 'swinging in the rain')

I think you should be using the the built in string operations which can be found here. Namely startswith(), endswith(), spilt() and rsplit().

You can also reassign variable names to give a more clear flow through the function. Here is my version of remove_prefix().

def remove_prefix(text, prefix, raise_if_no_match=True):
    if (text.startswith(prefix)):
        text = text.split(prefix, 1)[1]
    else:
        if (raise_if_no_match):
            msg_fmt = 'Text "{}" does not end with a prefix "{}".'
            raise ValueError(msg_fmt.format(text, prefix))
    return text

From that I'm sure you can change remove_suffix().

Also I would add docstrings to the functions so the arguments and exceptions raised are documented.


Efficient numpy array manipulation to convert an identity matrix to a permutation matrix Task

I want to be able to generate the permutation matrix that splits a 1D array of consecutive numbers (i.e. even, odd, even, odd, even, odd, ...) into a 1D array where the first half are the evens, and the second half are the odds. So (even1, odd1, even2, odd2, even3, odd3) goes to (even1, even2, even3, odd1, odd2, odd3).

For example, with N=6, the permutation matrix would be:

M = array([1, 0, 0, 0, 0, 0],
          [0, 0, 1, 0, 0, 0],
          [0, 0, 0, 0, 1, 0],
          [0, 1, 0, 0, 0, 0],
          [0, 0, 0, 1, 0, 0],
          [0, 0, 0, 0, 0, 1])

You can check that multiplying this with M * array([0, 1, 2, 3, 4, 5]) = array([0, 2, 4, 1, 3, 5]).

My approach in pseudocode

(Full code below.) This is the mathematically correct way to generate this:

I = NxN identity matrix
for i in [0:N-1]:
    if i < N/2:
        shift the 1 in row i by 2*i to the right
    if i >= N/2:
        shift the 1 in row i by 2*(i - N/2)+1 to the right

You can see how that works to generate M above.

Code (Python)

I implement the above pseudocode by using numpy array manipulation (this code is copy-and-pasteable):

import numpy as np

def permutation_matrix(N):
    N_half = int(N/2) #This is done in order to not repeatedly do int(N/2) on each array slice
    I = np.identity(N)
    I_even, I_odd = I[:N_half], I[N_half:] #Split the identity matrix into the top and bottom half, since they have different shifting formulas

    #Loop through the row indices
    for i in range(N_half):
        # Apply method to the first half
        i_even = 2 * i #Set up the new (shifted) index for the 1 in the row
        zeros_even = np.zeros(N) #Create a zeros array (will become the new row)
        zeros_even[i_even] = 1. #Put the 1 in the new location
        I_even[i] = zeros_even #Replace the row in the array with our new, shifted, row

        # Apply method to the second half
        i_odd = (2 * (i - N_half)) + 1
        zeros_odd = np.zeros(N)
        zeros_odd[i_odd] = 1.
        I_odd[i] = zeros_odd

    M = np.concatenate((I_even, I_odd), axis=0)

    return M

N = 8
M = permutation_matrix(N)
print(M)

Output:
array([[1., 0., 0., 0., 0., 0., 0., 0.],
       [0., 0., 1., 0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 1., 0., 0., 0.],
       [0., 0., 0., 0., 0., 0., 1., 0.],
       [0., 1., 0., 0., 0., 0., 0., 0.],
       [0., 0., 0., 1., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0., 1., 0., 0.],
       [0., 0., 0., 0., 0., 0., 0., 1.]])

My issues

I have a feeling that there are more efficient ways to do this. To summarise what I am doing to each matrix:

Looping through the rows
At each row, identify where the 1 needs to be moved to, call it idx
Create a separate zeros array, and insert a 1 into index idx
Replace the row we are evaluating with our modified zeros array

Is it necessary to split the array in two?

Is there an Pythonic way to implement two different functions on two halves of the same array without splitting them up?

Is there an approach where I can shift the 1s without needing to create a separate zeros array in memory?

Do I even need to loop through the rows?

Are there more efficient libraries than numpy for this?

Are there more efficient libraries than numpy for this?

Since permutation matrices are rather sparse, the scipy.sparse library is helpful.
Using its coo_matrix method we can build a matrix which contains given values at given indices.

From here it's just a matter of building the right lists of indices.

from itertools import chain
from scipy.sparse import coo_matrix

def permutation_matrix(n):
    # row and column indices - first even, then odd numbers in the latter
    I, J = range(n), list(chain(range(0, n, 2), range(1, n, 2)))
    # the following also works, if you are so inclined.
    # J = [m*2 + d for d, m in map(lambda k: divmod(k, n//2 + n % 2), range(n))]

    return coo_matrix(([1]*n, (I, J)))

If needed, we can use the .A property (short for .toarray()) to build a full matrix from this: e.g.
permutation_matrix(10).A.

Why bother with sparse matrices?

Multiplication with sparse matrices will be much faster: e.g. matrix-vector products can be computed in O(n) time instead of O(n^2).
Similarly, the memory requirements for storing these matrices in sparse format is O(n) instead of O(n^2).

  Is there an approach where I can shift the 1s without needing to
  create a separate zeros array in memory?

Sure, numpy.roll does circular shifting:

numpy.roll([1,0,0], 4) == numpy.array([0, 1, 0])

  Is there an Pythonic way to implement two different functions on two
  halves of the same array without splitting them up?

Not sure, but you can always cook something up.

from collections import deque
from itertools import chain

def apply_to_parts(part_selector, funs):

    def wrapper(vals):
        results = [deque() for _ in funs]

        for ix, val in enumerate(vals):
            part_ix = part_selector(ix, val)
            results[part_ix].append(funs[part_ix](val))

        return list(chain(*results))

    return wrapper

# multiply elements at even indices by 2
# divide   elements at odd  indices by 4
# return a list of elements in part 1 followed by elements in part 2
example = apply_to_parts(
    lambda ix, val: ix % 2,
    [lambda x:2*x, lambda y: y/4]
)

# should return [2, 6, 0.5, 1.0]
example([1,2,3,4])


Storing angles with overflow errors Rather than checking if my angle is in the range of 0 to 2pi every time it gets set, I got the idea to store it as an unsigned short with 0xFFFF being +2pi, thus the standard overflow behavior for unsigned numbers should keep it bound to the desired range.

Is it a good idea to do it this way, or is there something I'm missing?

#ifndef ANGLE_H
#define ANGLE_H
#include <cstdint>

class Angle
{
//static constexpr long double     _PI = 3.1415926535897932384626433832795028841971693993751058209749445923078164062862089986280348;
  static constexpr long double _TWO_PI = 6.2831853071795864769252867665590057683943387987502116419498891846156328125724179972560696;

    static inline __attribute((always_inline)) __attribute((pure))
    uint16_t uint16_from_double(const double a) { return (a * 0x00010000) / _TWO_PI; }
    uint16_t _theta;

    Angle(uint16_t theta) :
        _theta(theta)
    {
    }

public:
    Angle(double t = 0) :
        _theta(uint16_from_double(t));
    {
    }

    inline operator double() const
    {
        return radians();
    }

    inline double radians() const { return ((double) _theta) / 0x00010000) * _TWO_PI; }
    inline double degrees() const { return ((double) _theta) / 0x00010000) * 360; }

//everything is less than and greater than everything else, because it's a circle
//so return if subtracting will get us there faster than adding...
    inline bool operator<(const Angle & it) const
    {
        return _theta < it._theta? (it._theta - _theta) < 0x00008000 : (_theta - it._theta) >= 0x00008000;
    }
    inline bool operator<=(const Angle & it) const
    {
        return _theta < it._theta? (it._theta - _theta) <= 0x00008000 : (_theta - it._theta) > 0x00008000;
    }

    inline bool operator>(const Angle & it) const
    {
        return _theta < it._theta? (it._theta - _theta) > 0x00008000 : (_theta - it._theta) <= 0x00008000;
    }
    inline bool operator>=(const Angle & it) const
    {
        return _theta < it._theta? (it._theta - _theta) >= 0x00008000 : (_theta - it._theta) < 0x00008000;
    }

    inline Angle minDelta(const Angle & it) const
    {
        uint16_t i = _theta < it._theta? it._theta - _theta :  _theta - it._theta;
        return Angle(i < 0x00007FFF? i : 0x00010000 - i);
    }

    inline const Angle & operator=(double a)
    {
        _theta = uint16_from_double(a);
        return *this;
    }

    inline const Angle & operator+=(double a)
    {
        _theta += uint16_from_double(a);
        return *this;
    }

    inline const Angle & operator-=(double a)
    {
        _theta -= uint16_from_double(a);
        return *this;
    }

    inline const Angle & operator*=(double a)
    {
        _theta *= a;
        return *this;
    }

    inline const Angle & operator/=(double a)
    {
        _theta /= a;
        return *this;
    }
};

#endif // ANGLE_H

I find it amusing that you have written _TWO_PI out to 89 digits of precision. The ratio of the circumference of the observable universe relative to the size of a hydrogen atom is just 40 digits, which means that you couldn't possibly need any more precision than that for any physical calculation.  In any case, you're quantizing it to a uint16_t, which gives you less precision than a float, so even long double is overkill.

You could use one of these suggestions for the value of π.  In any case, you should never use an identifier with a leading underscore — names with one underscore are reserved for standard libraries, two underscores for the compiler.


C program to read data from text file and write it to a binary one This program will read data in the form of:

Number Name

In each line of a text file, then it will write it to a binary file and print the resulting binary file.

I am getting a warning in line 73, but I don't think it's an issue as null is converted into 0, right?

while(((fread((char*)&student, 1, sizeof(student), binary_file)) != NULL))

I'm talking about this line above

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define MAX_SIZE 50

void create_binary_file(const char *dest, const char *src);
void print_binary_file(const char *dest);

int main(void){
    const char ascii[MAX_SIZE] = "text.txt";
    char binary[MAX_SIZE];
    puts("Enter the name of the binary file");
    fgets(binary, sizeof(binary), stdin);
    binary[strlen(binary)-1] = '\0';

    create_binary_file(binary, ascii);
    print_binary_file(binary);

    return 0;
}

void create_binary_file(const char *dest, const char *src){
    struct student_tem{
        int ID;
        char name[MAX_SIZE];
    };

    struct student_tem student;

    FILE *text_file;
    FILE *binary_file;

    if((text_file = fopen(src, "r")) == NULL){
        perror(src);
        exit(EXIT_FAILURE);
    }

    if((binary_file = fopen(dest, "wb")) == NULL){
        perror(dest);
        exit(EXIT_FAILURE);
    }

    while(!feof(text_file)){
        if(1 != fscanf(text_file, "%i", &student.ID)){
            fprintf(stderr, "Error reading student number");
            exit(EXIT_FAILURE);
        }
        if(1 != fscanf(text_file, "%s", student.name)){
            fprintf(stderr, "Error reading student name");
            exit(EXIT_FAILURE);
        }
        fwrite(&student, 1, sizeof(student), binary_file);
    }

    fclose(text_file);
    fclose(binary_file);
}

void print_binary_file(const char *dest){
    struct student_tem{
        int ID;
        char name[MAX_SIZE];
    };

    struct student_tem student;
    FILE *binary_file;
    if((binary_file = fopen(dest, "rb")) == NULL){
        perror(dest);
        exit(EXIT_FAILURE);
    }

    while(((fread((char*)&student, 1, sizeof(student), binary_file)) != NULL)){
        printf("%i %s\n", student.ID, student.name);
    }
}

I am getting a warning in line 73, but I don't think it's an issue as null is converted into 0, right?
 while(((fread((char*)&student, 1, sizeof(student), binary_file)) != NULL)){

Well, if you look at the fread() reference documentation, you see that the return value is a simple size_t, not a pointer. NULL is meant to be a void* pointer and may be probably implemented like #define NULL ((void*)0) according to this.
C is still a strongly statically typed language, a ((void*)0) isn't the same as a ((size_t)0) hence the warning. And you should fix that, it actually is an issue.

Another thing you should look into is

 while(!feof(text_file)){

Why is iostream::eof inside a loop condition considered wrong?
It's merely the same problem in plain C code. You should rather check the streams state from the results of the fscanf() operations.


Representing database row as a class For my current project, I have elected to represent database rows as objects.

So an individual comment can be retrieved as a class like so:

$comment = new Comment(1);

Here is the Comment class code:

<?php
class Comment
{

    private $userid, $postid, $content, $date, $name, $ip, $legacy;

    public function __construct($comment_id)
    {
            $sql = new mysqli(REGISTRY_DBVALUES_SERVER, REGISTRY_DBVALUES_USERNAME, REGISTRY_DBVALUES_PASSWORD, REGISTRY_DBVALUES_DATABASE);

            $stmt = $sql->prepare("SELECT id, userid, postid, content, date, name, ip, legacy
                                   FROM ".SITE_TABLE_COMMENTS."
                                   WHERE id = ?");

            $stmt->bind_param("i", $comment_id);
            $stmt->bind_result($id, $userid, $postid, $content, $date, $name, $ip, $legacy);

            $stmt->execute();

            while($row = $stmt->fetch())
            {
                    $this->userid = $userid;
                    $this->postid = $postid;
                    $this->content = $content;
                    $this->date = $date;
                    $this->name = $name;
                    $this->ip = $ip;
                    $this->legacy = $legacy;
            }

            $stmt->free_result();
            $sql->close();
    }

    public function getUserID()
    {
            return $this->userid;
    }

    public function getPostID()
    {
            return $this->postid;
    }

    public function getContent()
    {
            return $this->content;
    }

    public function getDate()
    {
            return $this->date;
    }

    public function getName()
    {
            return $this->name;
    }

    public function getIP()
    {
            return $this->ip;
    }

    public function getLegacy()
    {
            return $this->legacy;
    }
}
?>

This allows you to retrieve the different attribute values for an individual comment using methods.

I am open to criticism on all aspects of the design, but was particularly curious if this is an accepted way of handling retrieval of individual data records?

This is not OOP, your comment is hard coupled with mysqli. What if you would write the exact same program but now with PDO? you will have to change the Comment class.
Then performance wise: you are now opening a mysqli connection for every comment.
You seem to think that using the word 'class' and method names as 'getMyVar()' equals OOP. It doesnt.
Programming is always about problem solving, and a good program is about solving a problem in such a way you can easily reuse that solution for other sort of the same problems.
OOP is very dangerous, because it alows us programmers to write even crapper code as before. That is why some really smart guys came up with loads of rules to live by in OOP land, called SOLID: http://en.wikipedia.org/wiki/SOLID_(object-oriented_design)
Now, lets place your code next to the SOLID rules.
1 Single responsibility principle
The Comment class fails on this account. It does multiple things as far as I can see. It works as a storage object for a Comment with methods to retrieve the parameters of the comment AND it also retrieves the comment from the Database. So we allready have a little problem here.
2 Open/closed principle
Again, not so good. An example: say I have a special sort of comment that has an extra field called 'permissions':
class PermissionComment extends Comment {}

Uh oh, In order to save the permission field into the object, I will have to rewrite the entire __construct while I only added 1 field. Not so good.
Also, all your variables are public variables. I don't need to use the get() methods to access a variable. I can access it directly. We thus violate the above rule in multiple ways. We cannot easily extend the comments functionality and we dont encapsulate it correctly
3 Liskov substitution principle
As I allready wrote somewhere at the top, say we want to change mysqli with PDO. Or we have written an extension of the mysqli object that does some caching of queries, I would now have to rewrite my Comment class because mysqli is hard coded into the object. Not so good again.
4 Interface segregation principle
This one, I will let go since it is not really in order here
5 Dependency inversion principle
Again, could be better. The Comment class i tightly coupled with your database connect. Your comment class 'knows' about the database-connection. It knows how to connect to a database. It knows how comments are stored while in fact it's just a container of comment data.
What now?
Back to the drawing board. You write you want to write a class that represents a Comment. Ok good. As example I omitted a lot of parameters in the code below, in fact, mostly because I'm lazy :p
So lets write a class that represents a basic Comment:
class Comment {

    private $id;

    private $content;

    private $title;

    public function __construct($data) {
        $this->id = $data['id'];
        $this->content = $data['content'];
        $this->title = $data['title'];
    }

    public function getId() {
        return $this->id;
    }

    public function getContent() {
        return $this->content;
    }

    public function getTitle() {
        return $this->title;
    }
}

Good, a Simple class that represents a Comment and only a Comment. But, one problem. What happend if we change the way the data is passed in. Or the key's change. and how can we be sure that that Comment class will only extract the data it needs. How does another programmer know what key's that $data array needs to have without having to read your entire code?
Let's change the constructor into:
public function __construct($content, $title) {
    $this->content = $content;
    $this->userId = $userId;
}

Ooeeh, cheecky. But huh? where did $id go?
Well, let's just say I want to create a Comment and later add it to the database, or I want to create a Comment object for testing purposes, or just a default comment to pass to the the frontend, my Comment doesn't have an id. I can now easily create a Content without needing a database id.
And for those Comments who do have an ID, we create an additional class:
class DbComment extends Comment {

    private $id;

    public function __construct($content, $userId, $id) {
        $this->id = $id;
        parent::__construct($content, $userId);
    }

    public function getId() {
        return $this->id;
    }
}

See how little code I had to write to simply add a Field to the Comment object?
But now your thinking: What a load of bull. This code can't even fetch a Comment from the database. True, but a 'Comment' object is a Comment and not a CommentFetcher.
We could now write a CommentFetcher class that fetches and saves Comments and returns a DbComment. Example:
class CommentFetcher {

    private $mysql;

    public function __construct(mysqli $mysql) {
        $this->mysql = $mysql;
    }

    public function fetch($id) {
        $stmt = $this->mysql->prepare("SELECT id, title, content FROM comment_comments WHERE id = ?");
        $stmt->bind_param("i", $id);
        $stmt->bind_result($id, $title, $content);
        $stmt->execute();
        return new DbComment($content, $title, $id);
    }
}

Note how i pass in a mysqli object in the constructor. All the class says is I need a mysqli object. If I now write my ThisClassCachesMysqliQueries extends Mysqli class I can simply pass it in, the CommentFetcher will have no idea I passed in a different class because it's still a mysqli object. But I will have added super duper caching into my program without having to change the CommentFetcher, cool huh?
So now we have a generic Comment class, we have a special case Comment, the DbComment. This is also a Comment but one that exists in a database somewhere and thus has an Id and we have a CommentFetcher that fetches comments from the Database.
Now a note: The code that I have written here is still crap. Don't use it ;)
A Class that can only fetch comments isn't really usefull. But you get the point, a CommentController that can update, fetch and insert Comments is something really nice.
Also I would advice you to rething how you are saving comments. Know, a comment has to know about users, ip, legacy,... While in fact all a Comment needs is a Title, data and content. A different table and controller would then look up hte user that postes the comment in the user_posted_comment table etc etc etc.
Once in a while, I take a step back an think, what am I writing? because at the end of the day, a Comment is a Comment and not some exotic objet that knows about users, ip-adresses and legacy.
But hey, atleast you are using bound params and prepared statements! Big up yoself.
I hope this answer helps you. else simply flame me in the commentbox below


Project Euler 34 # Digit Factorials in Python 145 is a curious number, as 1! + 4! + 5! = 1 + 24 + 120 = 145.
Find the sum of all numbers which are equal to the sum of the factorial of their digits.
Note: as 1! = 1 and 2! = 2 are not sums they are not included.

from time import time

def fact(n):
    """returns factorial of n."""
    total = 1
    for number in range(2, n + 1):
        total *= number
    return total

def fact_curious(n, factorials={1:1, 2:2}):
    """Assumes n is the range to check for curious numbers.
    generates curious numbers within n range."""
    for number in range(3, n):
        try:
            if sum(factorials[int(digit)] for digit in str(number)) == number:
                yield number
        except KeyError:
            for digit in str(number):
                factorials[int(digit)] = fact(int(digit))
            if sum(factorials[int(digit)] for digit in str(number)) == number:
                yield number

if __name__ == '__main__':
    start_time1 = time()
    print(sum(fact_curious(100000)))
    print(f'Time: {time() - start_time1}')

There is math.factorial function. So no need to reinvent it.
Using try/except for control flow is a bad practice. It's better to explicitly check whether a list contains a value using the condition if int(digit) in factorials. It also will eliminate the code duplication in except branch.
It's even better to pre-calculate all factorials for digits from 0 to 9. Anyway, you will reuse them all a lot of times.


Displaying a greeting to one user selected from a database I execute the following SQL statement to get back an data object into a variable.

  try {
    $getUser = $db->query('select * from user where user.user_id = '.$user_id);
  } catch (Exception $e) {
    echo $e->getMessage();
    die();
  }

from this I print out the information on the page where necessary e.g. the following:

  <?php
  foreach ($getUser as $user) {
    echo '

Welcome, '.$user['email'].'

    ';
  }
  ?>

The question is I know the size of the $getUser array to always be one.
Am I doing this the correct way when returning the information? Is there a better way where I do not need to use a foreach loop everytime.

The question is I know the size of the $getUser array to always be one.

No you don't. You assume it is - and it's probably a safe assumption to make. But actually knowing that user_id is a primary key is the job of the underlying database, not of your application.

You would know that 1 record is returned, if the query you send to the database says "give me one record where user_Id is equal to..." - if your backend is mysql that's done with a LIMIT clause.

Your query is missing a terminating semicolon... but the worst offense is the concatenation of the .$user_id into the WHERE clause. Where's that value coming from? Is there any slightest chance that it might not be containing the value you're expecting, and that concatenating it could result in unexpected SQL statements being injected into your query? If that's the case, then your query might actually be returning all rows, not just one.

See give me parameterized SQL, or give me death! (or many, many, many other online resources) for more details.


OpenGL first person jumping code I'm making a game in Java with LWJGL and I just implemented jumping. I don't really think the code is particularly well written or very efficient. Here is the code of my Camera class:

public void startJump(float height, float land){
    jumpHeight = height;
    jumpLand = land;
    if(!falling && !jumping)
        jumping = true;
}

private void jump(){ // called every frame
    if(jumping){
        if(y >= -jumpHeight){
            y -= jumpSpeed;
        }else{
            jumping = false;
            falling = true;
        }
    }

    if(falling){
        if(y <= -jumpLand){
            y += jumpSpeed;
        }else{
            jumping = false;
            falling = false;
        }
    }
}

If someone knows a cleaner or more efficient method please tell me!

EDIT:

I was thinking of using jumping and !jumping but this introduces a bug where if the jump key is held down then the player hovers in the air so unless your making a jet-pack use jumping and falling.

After your comment, the only change i could say is to change

private void jump(){ // called every frame
    if(jumping){
        if(y >= -jumpHeight){
            y -= jumpSpeed;
        }else{
            jumping = false;
            falling = true;
        }
    }

    if(falling){

to

private void jump(){ // called every frame
    if(jumping){
        if(y >= -jumpHeight){
            y -= jumpSpeed;
        }else{
            jumping = false;
            falling = true;
        }
    }

    else if(falling){

To avoid to check falling too if it's jumping. But its a macro optimization (useless too maybe in this case.)
But really, this code is perfect how it is. So go ahead!

Just got this idea in my mind: you could define a "PlayerStatus" enum with: Falling, Jumping but it will affect the logic of your game, change it only if you think you will need more status and maintain two boolean variables (or more if you add more conditions) will be hard. If you need only one turned on and others off.. it's a great way to do it:

enum PlayerStatus
{
    FALLING,
    JUMPING
}

and use the enum to change the status..

PlayerStatus status;
status = PlayerStatus.FALLING;


Simple Operations Calculator I'm practicing Swing effects. Everything works but for some reason I get a ton of error messages in the command prompt when the fields are empty, despite my attempt to add some exception handling (this has made me realize I need to practice that a lot more).

    /* Author: Luigi Vincent
Practicing Java Swing button and button events; Window that does simple calculation on two numbers
TO DO: Figure out why command prompt show so many messages when fields are empty & stop it
*/

import javax.swing.*;
import java.awt.*;
import java.awt.event.*;
import java.text.DecimalFormat;

public class SimpleCalc extends JFrame {

    SimpleCalc (String name) {
        super(name);

        setLayout(new FlowLayout());

        // Fields for input and output
        JTextField result = new JTextField(10);
        JTextField num1 = new JTextField(5);
        JTextField num2 = new JTextField(5);
        result.setEditable(false);

        // Buttons for operations
        JButton b_add = new JButton("Add");
        JButton b_sub = new JButton("Subtract");
        JButton b_mul = new JButton("Multiply");
        JButton b_div = new JButton("Divide");

        // Listener to handle operations
        ActionListener calculator = new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e) {
                try {
                    if (num1.getText().length() == 0 || num2.getText().length() == 0) {
                        throw new Exception();
                    }
                }
                catch (Exception ex){
                    JOptionPane.showMessageDialog(null, "Field missing", "Warning", JOptionPane.WARNING_MESSAGE);
                }

                Double alpha = Double.parseDouble(num1.getText());
                Double beta = Double.parseDouble(num2.getText());
                Double omega = null;
                DecimalFormat f = new DecimalFormat("##.00");

                if (e.getSource() == b_add) {
                    omega = alpha + beta;
                }
                else if (e.getSource() == b_sub) {
                    omega = alpha - beta;
                }
                else if (e.getSource() == b_mul) {
                    omega = alpha * beta;
                }
                else if (e.getSource() == b_div) {
                    omega = alpha / beta;
                }
                result.setText(f.format(omega).toString());
            }
        };
       // Register buttons to event listener
        b_add.addActionListener(calculator);
        b_sub.addActionListener(calculator);
        b_mul.addActionListener(calculator);
        b_div.addActionListener(calculator);

       // Add everything to frame + labels
        add(new JLabel("First Number"));
        add(num1);
        add(new JLabel("Second Number"));
        add(num2);
        add(new JLabel("Result"));
        add(result);
        add(b_add);
        add(b_sub);
        add(b_mul);
        add(b_div);
    }
    public static void main(String[] args) {
        SimpleCalc frame = new SimpleCalc("Simple Calculator");
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setSize(500, 100);
        frame.setLocationRelativeTo(null);
        frame.setVisible(true);
    }
}

I'm open to any and all improvements possible.

The major problem I can see is this piece of code :

            try {
                if (num1.getText().length() == 0 || num2.getText().length() == 0) {
                    throw new Exception();
                }
            }
            catch (Exception ex){
                JOptionPane.showMessageDialog(null, "Field missing", "Warning", JOptionPane.WARNING_MESSAGE);
            }

If you throw inside a try-catch, you will not exit the function. Since your condition is num1.getText().length() == 0 when you try to do Double.parseDouble(num1.getTest() well your string is empty so this throw an exception.

You should not use throw and catch to manage an error, not in this way. You should first remove the try-catch and work with a simple if :

if (num1.getText().isEmpty() || num2.getText().isEmpty()) {
    JOptionPane.showMessageDialog(null, "Field missing", "Warning", JOptionPane.WARNING_MESSAGE);
    throw new Exception();
}

I would not suggested that since now you add a complexity to your code.

The method now need to declare throws Exception (which is not possible for actionPerformed()), now the caller need to manage this possible outcome.
Exception would be too generic.
I guess that this case would happen a lot,so throwing an Exception could become heavy for the program.

The real solution is to only return from the method. You've already warned the user that he need to do something (you could be more clear) and you don't want to execute the rest of method.

There is some general notion that are lacking in your code, but I won't cover it all, since I mention it in others answer :

There is naming convetions in Java, b_add does not follow it, it should be bAdd (but it could simply be add)
You should not extend JFrame, but rather just have a private JFrame, you're not adding any functionnality to JFrame you're only using it. (Composition over inheritance) answer that cover a bit that part


Using IServiceScopeFactory without implementing the Service Locator Pattern I want to use IServiceScopeFactory in a way, which is not the Service Locator anti-pattern.

Therefore I thought about making a generic one, which supports explicitly only the given service.

I have made these interfaces:

public interface IServiceScopeFactory<T> where T : class
{
    IServiceScope<T> CreateScope();
}

public interface IServiceScope<T> : IDisposable where T : class
{
    T Service { get; }
}

And these implementations:

public class ServiceScopeFactory<T> : IServiceScopeFactory<T> where T:class
{
    private readonly IServiceScopeFactory _serviceScopeFactory;

    public ServiceScopeFactory(IServiceScopeFactory serviceScopeFactory)
    {
        _serviceScopeFactory = serviceScopeFactory;
    }

    public IServiceScope<T> CreateScope()
    {
        return new ServiceScope<T>(_serviceScopeFactory.CreateScope());
    }
}

public class ServiceScope<T> : IServiceScope<T> where T : class
{
    private readonly IServiceScope _scope;
    private T _service;

    public ServiceScope(IServiceScope scope)
    {
        _scope = scope;
    }

    public void Dispose()
    {
        _scope?.Dispose();
    }

    public T Service => _service ?? (_service = _scope.ServiceProvider.GetRequiredService<T>());
}

Usage is for example:

public class Foo
{
    private readonly IServiceScopeFactory<ConmaniaDbContext> _dbContextFactory;

    public Foo(IServiceScopeFactory<ConmaniaDbContext> dbContextFactory)
    {
        _dbContextFactory = dbContextFactory;
    }

    public void Bar()
    {
        using (var scope = _dbContextFactory.CreateScope())
        {
            var dbContext = scope.Service
            //use service
        }
    }
}

For registering it, I have made these extension methods:

public static void AddServiceScopeFactory<T>(this IServiceCollection serviceCollection) where T : class
{
    serviceCollection.AddTransient<IServiceScopeFactory<T>, ServiceScopeFactory<T>>();
}

The registration is like that:

services.AddScoped<ConmaniaDbContext, ConmaniaDbContext>(); //Scoped Service
services.AddServiceScopeFactory<ConmaniaDbContext>();
services.AddSingleton<Foo, Foo>(); //Singleton Service

My base problem was that I need the Scoped Service DbContext in a Singleton service.

Is this a good way for dependency injection with a custom scope or is this just a wrapper around the Service Locator pattern? Is there anything you would improve? Or any obviously problems I might get with this implementation?

To clarify, this is where I am coming from:

public class Foo
{
    private readonly IServiceScopeFactory _scopeFactory;

    public Foo(IServiceScopeFactory scopeFactory)
    {
        _scopeFactory = scopeFactory;
    }

    public void Bar()
    {
        using (var scope = _scopeFactory.CreateScope())
        {
            var dbContext = scope.ServiceProvider.GetRequiredService<ConmaniaDbContext>()
            //use service
        }
    }
}

But this feels like the Service Locator Pattern, or am I wrong? The problem is here, that the class constructor doesn't know which dependencies the class needs.

The problem I am facing is that IServiceScopeFactory feels like the Service Locator Pattern. Therefore I created a IServiceScopeFactory<T>.

Since the whole Dependency Injection Pattern is relative new to me, I want to know if this is a suitable way of doing it, or some misconception.

I'm of the opinion that this is awesome, and I have a couple of suggestions to add. It's not "Service Locator" because you're explicitly constraining to the exact type that can be resolved---the explicit dependency is right up there in the constructor.

(I wish they'd bake this into .Net Core.)

Don't store the service-object

I think it would be better to replace T Service with T GetRequiredService(). If you're resolving a Transient service, it would be useful. Additionally, it makes for fewer changes when someone switches away from the built-in IServiceScopeFactory.

Finish implementing IDispose

See this answer on StackOverflow

Go Singleton

I see no reason to register as Scoped, considering what this does. It's using a Singleton, so make it Singleton.

Here's my tweaked version

public interface IServiceScopeFactory<T> where T : class
{
    IServiceScope<T> CreateScope();
}

public interface IServiceScope<T> : IDisposable where T : class
{
    T GetRequiredService();
    T GetService();
    IEnumerable<T> GetServices();
}

public class ServiceScopeFactory<T> : IServiceScopeFactory<T> where T : class
{
    private readonly IServiceScopeFactory _serviceScopeFactory;

    public ServiceScopeFactory(IServiceScopeFactory serviceScopeFactory) => _serviceScopeFactory = serviceScopeFactory;

    public IServiceScope<T> CreateScope() => new ServiceScope<T>(_serviceScopeFactory.CreateScope());
}

public class ServiceScope<T> : IServiceScope<T> where T : class
{
    readonly IServiceScope _scope;

    public ServiceScope(IServiceScope scope) => _scope = scope;

    public T GetRequiredService() => _scope.ServiceProvider.GetRequiredService<T>();

    public T GetService() => _scope.ServiceProvider.GetService<T>();

    public IEnumerable<T> GetServices() => _scope.ServiceProvider.GetServices<T>();

    #region IDisposable/Dispose methods ( https://stackoverflow.com/a/538238/530545 )
    bool _disposed = false;

    public void Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }

    protected virtual void Dispose(bool calledFromCodeNotTheGarbageCollector)
    {
        if (_disposed)
            return;
        if (calledFromCodeNotTheGarbageCollector)
        {
            // dispose of manged resources in here
            _scope?.Dispose();
        }
        _disposed = true;
    }

    ~ServiceScope() { Dispose(false); }
    #endregion

}

You'd register this like so:

services.AddSingleton(typeof(IServiceScopeFactory<>), typeof(ServiceScopeFactory<>));

And to work with your example, you'd use it like so (assuming you registered your ConmaniaDbContext):

public class Foo
{
    readonly IServiceScopeFactory<ConmaniaDbContext> _dbCtxFactory;

    public Foo(IServiceScopeFactory<ConmaniaDbContext> dbCtxFactory) => _dbCtxFactory = dbCtxFactory;

    public void Bar()
    {
        using var scope = _dbCtxFactory.CreateScope();
        var db = scope.GetRequiredService();
        //use service
    }
}

FYI/Backstory

FYI, I'm using this in a .Net Core 3.x app that's leveraging Gql.Net. This pattern makes it so I can actually use EFC. Gql.Net's field resolvers run in parallel, so if you don't do something like this, you cannot use your constructor-injected DbContext in more than one field, or you wind up getting this EFC exception (which is not unique to Gql.Net):

  [InvalidOperationException] A second operation started on this context
  before a previous operation completed. This is usually caused by
  different threads using the same instance of DbContext. For more
  information on how to avoid threading issues with DbContext, see
  https://go.microsoft.com/fwlink/?linkid=2097913.

I've confirmed this solves that "parallel execution in the same Http request" issue for me. I can now run multiple, concurrent tasks against the database from the same instance of a scoped service class. It's because with this solution, instead of all threads using the same DbContext that was injected into the constructor, they now each resolve their own, scoped DbContext instance when they need it.


Get values between two dates I have a search form where users can enter ValidFrom and ValidTo dates,

There are three conditions:

ValidFrom and ValidTo NOT NULL

Action: Get values between two dates
ValidFrom is NOTNULL and ValidTo is NULL

Action: Get values where date is greater than ValidFrom
ValidFROM is NULL and ValidTo is NOTNULL

Action: get values where date is less than ValidTo

This is what I have got:

   if (model.ValidFrom != null && model.ValidTo != null)
        {
            var dateFrom = (DateTime)model.ValidFrom;
            var dateTo = (DateTime)model.ValidTo;
            dalList = dalList.Where(s =>
                DbFunctions.TruncateTime(s.ValidFrom) >= dateFrom.Date
                && DbFunctions.TruncateTime(s.ValidTo) <= dateTo.Date).AsQueryable();
        }
        if (model.ValidFrom != null && model.ValidTo == null)
        {
            var dateFrom = (DateTime)model.ValidFrom;

            dalList = dalList.Where(s => DbFunctions.TruncateTime(s.ValidFrom) >= dateFrom.Date).AsQueryable();
        }
        if (model.ValidFrom == null && model.ValidTo != null)
        {
            var dateTo = (DateTime)model.ValidTo;
            dalList = dalList.Where(s => DbFunctions.TruncateTime(s.ValidTo) <= dateTo.Date).AsQueryable();
        }

Is there a better way to write this type of search?

Although the answer from Heslacher is a really good one, here's also what you might do. Create two boolean variable to check for null on the model.ValidFrom and model.ValidTo:

bool modelValidFromIsNull = model.ValidFrom == null;
bool modelValidToIsNull = model.ValidTo == null;

Now, like Heslacher mentioned, place a guard to rule out if they are both null.

if (modelValidFromIsNull && modelValidToIsNull) { return null; }

Now you can use these bool variables to do some magic. In the Where() clause of the query, use these flags to check or ignore the condition you want to apply. If the flag is true, the condition will be skipped and if it is false, the condition will be evaluated. A small example:

var numbers = new[] { 1, 2, 3, 4, 5, 6 };
var remaining = numbers.Where(x => false || x > 3);

The variable remaining will now contain 4, 5 and 6. If you were to change that false to true, the variable remaining would contain all the numbers from the original source. [That's why you need the guard :) ]

Now you can apply this logic to your code. When the model.ValidFrom is not null, your variable modelValidFromIsNull will be false. This means that in the where clause the left part is false and the second part will be evaluated. The same goes for model.ValidTo.

This is the resulting code:

bool modelValidFromIsNull = model.ValidFrom == null;
bool modelValidToIsNull = model.ValidTo == null;

if (modelValidFromIsNull && modelValidToIsNull) { return null; }

var result = dalList.Where(s => modelValidFromIsNull || DbFunctions.TruncateTime(s.ValidFrom) >= dateFrom.Date)
                    .Where(s => modelValidToIsNull || DbFunctions.TruncateTime(s.ValidTo) <= dateTo.Date);


Binary Search Tree without pass by reference Here is a code for BST that I have written , that does not use pass by reference. Help me to find flaws in its design. Also I do not like the function getReqNodes, can it be made more elegant.
#include <iostream>
using namespace std;

struct node {
    int data;
    node* left = nullptr;
    node* right = nullptr;
    node(int d , node *l = nullptr, node *r = nullptr){
        data = d;
        left = l;
        right = r;
    }
};
node* insertVal(node* n, int val) {
    if (n == nullptr) {
        n = new node(val);
        n->data = val;
        return n;
    }
    else if (val < n->data){
        return new node(n->data, insertVal(n->left,val), n->right);
    }
    else if(val > n->data) {
        return new node(n->data, n->left, insertVal(n->right, val));
    }
    else{
        return new node(val, n->left, n->right);
    }
}
int minVal(node* n) {
    if (n == nullptr) return -10000;
    else if (n->left == nullptr) return n->data;
    else return minVal(n->left);
}

node* getReqNodes(node *t, node *q){
    if(q == nullptr){return t;}
    else{
        t = insertVal(t,q->data);
        t = getReqNodes(t,q->left);
        t = getReqNodes(t,q->right);
        return t;
    }
}

node* removeVal(node *n, int v){
    if (n == nullptr){return n;}
    else if(v < n->data){
        return new node(n->data, removeVal(n->left,v), n->right);
    }
    else if(v > n->data){
        return new node(n->data, n->left, removeVal(n->right, v));
    }
    else{
        node *j = n;
        //attaching right subtree to the left subtree
        node *p = getReqNodes(n->left, n->right);
        delete j;
        return p;
    }
}

void printTree(node* n) {
    if (n == nullptr) { return; }
    printTree(n->left);
    cout << n->data << " ";
    printTree(n->right);
}

int main() {
    node* p = nullptr;
    for(int i = 0; i < 15; ++i){
        p = insertVal(p, rand()%15);
    }
    printTree(p);cout<<endl;cout<<endl;
    node *foo = removeVal(p, 149);
    printTree(foo);
    return 0;

}

General Observations
The code compiles and runs in Visual Studio 2019 on Windows 10.
The vertical spacing is inconsistent and could be improved.
The code could be reusable if the Binary Search Tree was made into a class.
The unit testing in main could be more effective if know values were used first and then a second test with random number generated. The test of removeVal() currently only tests a value that is never in the BST, the function does work because I changed the value in the test.
Avoid using namespace std;
If you are coding professionally you probably should get out of the habit of using the using namespace std; statement. The code will more clearly define where cout and other identifiers are coming from (std::cin, std::cout). As you start using namespaces in your code it is better to identify where each function comes from because there may be function name collisions from different namespaces. The identifiercout you may override within your own classes, and you may override the operator << in your own classes as well. This stack overflow question discusses this in more detail.
Function and Variable Naming
Generally the function names are fairly good, there is one exception, it isn't clear to me from the name what getReqNodes() is supposed to do.  In main it might be better to have a variable called bst or root rather than p.
Random Number Generation
It would be better to use C++ random number generators rather than the old C library function rand() function, see EDIT 2 in the first answer to this stack overflow question. Currently the random number generator is not random because it is never seeded with a value by calling srand().
One Statement Per Line
The code contains multiple lines where there is more than one statement such as:
    printTree(p); cout << endl; cout << endl;

This makes the code harder to read, understand and maintain, always remember that you may not be the only person that needs to edit the code.
There are 2 other issues with that line, that std::endl flushes the output which affects the performance of the program so using:
    std::cout << "\n";

might be a better option. The second is that if a string such as "\n" is used then the string "\n\n" can be used to output 2 new lines, decreasing the number of statements necessary, although this is also possible:
    std::cout << std::endl << std::endl;


simplifying regex expression for timestamp I was wondering if it's possible to simplify this regex string (JS):
function validateDateString(date){
    const regex = /[0-1][0-9]-[0-3][0-9]-[0-9]{4} [0-2][0-9]:[0-5][0-9] [A|P][M]/;
    return regex.test(date)
}

It is supposed to validate a string in the following format: MM-DD-YYYY hh:mm A

Some minor tips:

Add "^" and "$" to your regex to ensure your input string doesn't contain anything but a date
No brackets are needed around the [M] - A simple M means the same thing
Instead of checking for AM/PM with [A|P][M], keep the AM/PM characters together, like this: (AM|PM) - it makes it much more readable.
Since it's impossible to accurately test numeric ranges of a date-string in regex, I wouldn't even attempt it. If you really want to ensure the numbers fall into a valid range, do so after the fact, outside of the regex. Inside the regex, you can simply use \d to match any digit.

After applying these tips, you'll be left with a pretty simple regex that looks like this:
/^\d\d-\d\d-\d\d\d\d \d\d:\d\d (AM|PM)$/

As an aside, you might notice it's possible to remove a little repetition in the regex with \d{2} or \d{4}, but don't bother. There's not really a lot of repetition, and the {2}/{4} will just obfuscate what you're trying to accomplish here.
Depending on your use case, simply using the above regex might be good enough, but if you also want to ensure the numbers fall into a valid range, I would actually parse the numbers out of the date-string, feed them into Javascript's Date class, and verify the numbers using the date instance. Relying on the built-in Date class will help with edge cases, such as leap years. In the below code snippet I am assuming that these date-strings are in UTC

function validateDateString(dateStr) {
  const regex = /^(\d\d)-(\d\d)-(\d\d\d\d) (\d\d):(\d\d) (?:AM|PM)$/;
  const match = regex.exec(dateStr);
  if (!match) return false;

  const [month, day, year, hr, min] = match.slice(1).map(Number);
  const date = new Date(Date.UTC(year, month-1, day, hr, min));
  return (
    year === date.getUTCFullYear() &&
    month === date.getUTCMonth() + 1 &&
    day === date.getUTCDate() &&
    hr === date.getUTCHours() &&
    min === date.getUTCMinutes()
  );
}

console.assert(validateDateString('11-30-1999 12:34 AM'));
console.assert(!validateDateString('13-30-1999 12:34 AM'));
console.assert(!validateDateString('1x-30-1999 12:34 AM'));
console.log('Done')

The important thing to understanding how the above code snippet works, is to understand what happens when you give new Date() (or in this case, date.UTC()) a too-big number. If, for example, you passed in 5 for the hours and 70 for the minutes, then it'll normalize the values by moving an hour worth of time from the minutes to the hours, leaving it with 6 as the hours and 10 as the minutes.
Here's what this looks like in action:
> const d = new Date(0, 0, 0, 5 /* hours */, 70 /* minutes */)
> d.getHours()
6
> d.getMinutes()
10

In the above example, it's easy enough to test that a numeric overflow happened, we just need to compare our input numbers with our outputs. We can quickly see that 5 !== 6 and 70 !== 10, so some sort of overflow must have happened, which means our date string was not valid. This is the same principle being used in the above code snippet - we just test each input with each output to ensure nothing overflowed.


Test blank cell and out put phonetic (text) I have this code which first checks for blank cell in Column B, and then blank cells in Column F. If the results is Yes-No, then it prints the phonetic into F.

Can someone review the code, please? I am also wondering if there is a way to set the starting point to be Column B (therefore, Cells(i,1)=B1) and possibility replace Cells(i,2) by dim as something(long?).

Sub test()

'Dim rng As Range
Dim i As Long

'Set rng = Range("B:B")
i = 1

Do Until IsEmpty(Cells(i, 2))

    If Cells(i, 2).Value <> "" Then
        If Cells(i, 6).Value = "" Then
        Cells(i, 2).Offset(0, 4).Value = Cells(i, 2).Phonetic.Text
        End If
    End If
    i = i + 1
Loop

End Sub

Give your code a good name - not test - it doesn't tell me what it's doing.

At the same time, it's implicitly public so you might as well put that and always turn on Option Explicit. You can have it automatically by going to Tools -> Options in the VBE and checking the Require Variable Declaration option. This way if you have any variables not defined, the compiler will let you know.

You can use Do Until isEmpty but why not find the last cell and use a For loop? There is a standard way to find lastRow and lastColumn. That post explains why.

Technically you could eliminate If Cells(i, 2) <> vbNullString because the lastRow should determine that, but so would your Do Until isEmpty so it's already repetitive. You can also avoid assigning i an initial value and get rid of your messy (for VBA) iteration.

I would also eliminate the if block, personally.

Option Explicit

Public Sub GetPhonetic()
    Dim lastRow As Long
    lastRow = Cells(Rows.Count, 2).End(xlUp).Row
    Dim i As Long
    For i = 1 To lastRow
        If Cells(i, 6) <> vbNullString Then Cells(i, 6) = Cells(i, 2).Phonetic.Text
    Next i
End Sub

Also eliminate the commented code - if you aren't using it, it's just noise. I also eliminated the extra white space as you don't need it.

Re:

  I am also wondering if there is a way to set the starting point to be
  Column B (therefore, Cells(i,1)=B1) and possibility replace Cells(i,2)
  by dim as something(long?)?

I'm not sure what you mean here, you are already dim i as long and using that to determine your starting point; there's no need for it to be more complicated than cells(i,y).


Replace multiple occurrences of a character I try to replace multiple occurrences of a character with a single character.

Input:

  hhiii!!!  hooowww   aaareee   yyyooou???

Output:

  hi! how are you?

public class CharSingleOccurrence {
    public static void main(String... args){
        charSingleOccurrence("hhiii!!!  hooowww   aaareee   yyyooou???");
    }
    public static void charSingleOccurrence(String str){
        int i=0;
        int j=0;
        char arr[]=new char[50];
        while (i<str.length()-1) {
            if(str.charAt(i)!=str.charAt(i+1)){
                arr[j]=str.charAt(i);
                i++;
                j++;
            }else{
                i++;
            }
        }
        arr[j]=str.charAt(str.length()-1);
        for(char c:arr){
            System.out.print(c);
        }
    }
}

Here I need review for

char arr[]=new char[50];
arr[j]=str.charAt(str.length()-1);

Maybe I am wrong but I prefers to use the regex for this kind of text manipulation.
Here is my code,

System.out.println("hhiii!!!  hooowww   aaareee   yyyooou???"
                      .replaceAll("(.)\\1+","$1"));


Practicing HTML Tables I am trying to learn by doing. Here is the first problem that I have solved. I have actually not done it perfectly. The table header should cover both the text and the image but it is only above text. If you can help me out with the design I will be thankful.

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">

<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<title>Untitled Document</title>

Food you love to eat

                I love eating food . All types of food are good for health. I like fruits in them. Fruits are good for health.

The HTML you submitted was invalid. I changed the following things so your html would be valid:

Don't put
 tags inside a table. Header tags are used for headings only. Don't use headings to make text BIG or bold. Use CSS for positioning and font sizing.
In a table, the
 tag is the first tag that should appear.
In the  tag, the attribute alt is required. The alt attribute specifies alternate text for an image if the image cannot be displayed.

Your HTML validated:

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">

<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<title>Untitled Document</title>

Food you love to eat

                I love eating food . All types of food are good for health. I like fruits in them. Fruits are good for health.


Excel VBA - Get Folder Names I need to get folder names with the path for which I need to search the directory in a server with about 6000 folders. I have the following snippet of code to run through the folder and get the folder names with path. It works fine in a local directory but when I run the same code on a server directory it fails after about printing 86 folder names. The code works absolutely fine but stops working when run on a server directory with about 6000 folders. Excel freezes up and seems like it hangs.

Private Sub PrintFolders()
Dim objFSO As Object
Dim objFolder As Object
Dim objSubFolder As Object
Dim i As Integer
Application.StatusBar = ""
'Create an instance of the FileSystemObject
Set objFSO = CreateObject("Scripting.FileSystemObject")
'Get the folder object
Set objFolder = objFSO.GetFolder("C:\Temp")
i = 1
'loops through each folder in the directory and prints their names and path
On Error GoTo handleCancel
Application.EnableCancelKey = xlErrorHandler
MsgBox "This may take a long time: press ESC to cancel"
For Each objSubFolder In objFolder.subfolders
Application.StatusBar = objSubFolder.Path & " " & objSubFolder.Name
    'print folder name
    Cells(i + 1, 1) = objSubFolder.Name
    'print folder path
    Cells(i + 1, 2) = objSubFolder.Path
    i = i + 1
Next objSubFolder
handleCancel:
If Err = 18 Then
 MsgBox "You cancelled"
End If
End Sub

UX
Before diving into the code, I have a few points to mention about the user experience of it.

You're not resetting the Application.Statusbar before the procedure exits, which means whenever the code runs to completion, the Excel status bar remains "frozen" with the last processed folder.
You're warning the user that the operation may take a long time, regardless of how many folders/subfolders there actually are.
Message boxes are bare-bones, without a title or an icon, and punctuation is missing from the message strings.

Readability
The code itself reads a bit like a clogged script. Indentation is insufficient and inconsistent, and vertical whitespace is completely inexistent. This code needs to breathe a little - here's your code, without any other changes:
Private Sub PrintFolders()

    Dim objFSO As Object
    Dim objFolder As Object
    Dim objSubFolder As Object
    Dim i As Integer

    Application.StatusBar = ""

    'Create an instance of the FileSystemObject
    Set objFSO = CreateObject("Scripting.FileSystemObject")
    'Get the folder object
    Set objFolder = objFSO.GetFolder("C:\Temp")

    i = 1
    'loops through each folder in the directory and prints their names and path
    On Error GoTo handleCancel

    Application.EnableCancelKey = xlErrorHandler
    MsgBox "This may take a long time: press ESC to cancel"

    For Each objSubFolder In objFolder.subfolders
        Application.StatusBar = objSubFolder.Path & " " & objSubFolder.Name
        'print folder name
        Cells(i + 1, 1) = objSubFolder.Name
        'print folder path
        Cells(i + 1, 2) = objSubFolder.Path
        i = i + 1
    Next objSubFolder

handleCancel:
    If Err = 18 Then
        MsgBox "You cancelled"
    End If

End Sub

Comments
There are way too many comments in that code. Good, valuable comments explain why code is doing what it does - the code itself should be self-explanatory about the what.
I would simply remove them... all.

'Create an instance of the FileSystemObject
Set objFSO = CreateObject("Scripting.FileSystemObject")

Isn't too far from:
'increment i:
i = i + 1

Non-Responsiveness
You're running a pretty tight loop here:
For Each objSubFolder In objFolder.subfolders

    Application.StatusBar = objSubFolder.Path & " " & objSubFolder.Name
    Cells(i + 1, 1) = objSubFolder.Name
    Cells(i + 1, 2) = objSubFolder.Path

    i = i + 1

Next objSubFolder

You never give Excel a chance to breathe and actually respond to the events you're sending - namely updating the statusbar and listening to ESC keypresses.
This would fix it:
For Each objSubFolder In objFolder.subfolders

    Application.StatusBar = objSubFolder.Path & " " & objSubFolder.Name
    Cells(i + 1, 1) = objSubFolder.Name
    Cells(i + 1, 2) = objSubFolder.Path

    i = i + 1
    DoEvents

Next objSubFolder

...but it comes with a pretty high price: updating the status bar and listening for keypresses at each and every iteration will considerably slow down execution. How about reorganizing it a little, and only do that once every 10 iterations? Avoid magic numbers, give that value a meaningful name and assign it to a constant:
Const IterationsToUpdate As Integer = 10
For Each objSubFolder In objFolder.subfolders

    Cells(i + 1, 1) = objSubFolder.Name
    Cells(i + 1, 2) = objSubFolder.Path

    i = i + 1

    If i Mod IterationsToUpdate = 0 Then
        Application.StatusBar = objSubFolder.Path & " " & objSubFolder.Name
        DoEvents
    End If

Next objSubFolder

Now, you're writing to worksheet cells in a loop, while keeping Excel relatively responsive. Are you sure you're writing to the correct worksheet?

Cells(i + 1, 1) = objSubFolder.Name
Cells(i + 1, 2) = objSubFolder.Path

Without an object reference, Cells is referring to the active sheet, which the user is free to change any time as the code runs. This is another UX issue if not a bug.

Performance
I'm willing to bet anything that your biggest bottleneck isn't the FSO, but actually writing to the cells. Of course you need to do that.. but do you need Excel to repaint its grid and verify that nothing needs to be recalculated every time you write to a cell? Of course you don't.
Switch it off:
Application.ScreenUpdating = False
Application.Calculation = xlCalculationManual

Now, whenever you do that, you must handle runtime errors appropriately.

Error Handling
Your error handling is copy-pasted from MSDN, which is only an example to illustrate how a specific feature works - these examples are always focused on a very specific feature, and never about best practices.
Your method should fail cleanly, and correctly cleanup whether or not an error is raised - every error-sensitive method should be templated something like this:
Sub DoSomething()
    On Error GoTo CleanFail

    'implementation code here

CleanExit:
    'cleanup code here
    Exit Sub

CleanFail:
    'error-handling code here
    Resume CleanExit
End Sub

In your case, it would look something like this:
CleanExit:
    Application.Statusbar = False
    Application.Cursor = xlDefault
    Application.ScreenUpdating = True
    Application.Calculation = xlCalculationAutomatic
    Exit Sub

CleanFail:
    Const MsgTitle As String = "Operation not completed"
    If Err.Number = 18 Then
        MsgBox "Operation was cancelled.", vbInformation, MsgTitle
    Else
        MsgBox "An error has occurred: " & Err.Description, vbCritical, MsgTitle
    End If
    Resume CleanExit

Abstraction & Maintainability
If you need to change the folder to run it for, you need to change the code: it's screaming for a String parameter containing the root path to start searching in.
The procedure is also doing too many things. To add features you need to add more code to it, making it do even more things.
Consider these concerns:

Knowing what folder to start searching in
Acquiring the folders/subfolders' names
Warning the user about a potentially lengthy operation
Enabling/disabling/toggling screen updating, calculation, statusbar and cursor
Knowing what worksheet to output to
Dropping the folders/subfolders' names onto the output worksheet

Most of these concerns deserve a method of their own. Here's how I would write your code:
Option Explicit

Public Sub RenameMe()

    Dim rootPath As String
    rootPath = GetRootPath
    If rootPath = vbNullString Then Exit Sub

    Dim folders() As Variant
    Set folders = FindAllFolders(rootPath)

    Dim targetSheet As Worksheet
    Set targetSheet = Application.ActiveSheet

    OutputFolders targetSheet, folders

End Sub

Split concerns into small, immutable procedures - a procedure should ideally have no more than one reason to change. Doing that increases the abstraction level of your code, which instantly boosts readability, which in turn automatically reduces the possibility for hidden bugs.

Algorithm
If you noticed, the above snippet separates getting the folders and writing them to a worksheet. You may think "but I'll have to iterate all 6,000 folders twice, this is going to be so much slower!"... but I'll give you a hint: read up on Arrays and Ranges in VBA (bookmark that site!).
The crux is that you don't need to iterate anything other than the folders: you populate a 2D array as you go, and then write that entire array onto the worksheet in a single operation. Then your performance bottleneck will be the FileSystemObject, but at that point your code will be so unrecognizeable that it'll be worth posting another Code Review question! :)


Long-term archiving + encrypting shell script Yesterday, I very quickly, in an hour, wrote the very first version of my long-term archiving + encrypting shell script.

I am aware it may have too many flaws, but none of them really prevent it from normal operation, so before I take another step and restructure it with printf and better error reporting, structure, exit codes, etc. I would like you to have a look first.

What it does:

It archives a given directory into a tarball.
It compresses (with maximum effort) that tarball using xz.
It encrypts that xz archive using openssl and AES-256.
It creates and checks the created SHA-512 sum file.
It then decrypts the created file to a temporary file.
Finally, it compares the decrypted file with the original xz archive.

I am prepared to waste time and effort in order to be 100% sure before burning my precious data to Blu-ray M-disc.

I know I should not have to do steps 5 and 6 just because there is almost a 0% chance for the data to get corrupted in the process. But since I have a rather large NVMe SSD drive, and relatively fast CPU, it should not be so shocking that I wrote it with motto better safe than sorry.

#!/bin/sh

[ -z "$1" ] && echo "You need to give me one directory!" && exit 1

[ ! -d "$1" ] && echo "This is not a directory!" && exit 1

[ ! -w "$PWD" ] && echo "The current directory is not writable by you!" && exit 1

dir=$(basename "$1")
today=$(date +%Y-%m-%d)

backupdir="${dir}_${today}"
# backupdir="${dir}"

backupfile="${backupdir}.tar"

bold=$(tput bold)
red=$(tput setaf 1)
yellow=$(tput setaf 3)
nocolor=$(tput sgr0)
bold_red="$bold$red"
bold_yellow="$bold$yellow"

echo
echo "${bold_yellow}1/6: TAR is archiving your directory '${bold_red}$dir${bold_yellow}' into '${bold_red}$backupfile${bold_yellow}'${nocolor}"
echo

if ! tar -cf "$backupfile" "$1" --totals
then
    echo "TAR failed to archive your directory"
    exit 1
fi

echo
echo "${bold_yellow}2/6: XZ is compressing your '${bold_red}$backupfile${bold_yellow}' into '${bold_red}$backupfile.xz${bold_yellow}'${nocolor}"
echo

if ! xz --format=xz --check=sha256 -9 --threads=8 --keep --verbose --verbose --extreme "$backupfile"
then
    echo "XZ failed to compress your backup file"
    exit 1
fi

echo
echo "${bold_yellow}3/6: OpenSSL is encrypting your '${bold_red}$backupfile.xz${bold_yellow}' into '${bold_red}$backupfile.xz.enc${bold_yellow}'${nocolor}"
echo

if ! pv -W "$backupfile.xz" | openssl enc -aes-256-cbc -md sha256 -salt -out "$backupfile.xz.enc" -pass file:/home/vlastimil/.blablablabla 2> /dev/null
then
    echo "OpenSSL failed to encrypt your compressed file"
    exit 1
fi

echo
echo "${bold_yellow}4/6: sha512sum is creating hash file '${bold_red}$backupfile.xz.enc.SHA512SUM${bold_yellow}'${nocolor}"
echo

if ! sha512sum -b "$backupfile.xz.enc" > "$backupfile.xz.enc.SHA512SUM"
then
    echo "sha512sum failed to compute hashsum file"
    exit 1
fi

sha512sum -c "$backupfile.xz.enc.SHA512SUM"

echo
echo "${bold_yellow}5/6: Decrypting your '${bold_red}$backupfile.xz.enc${bold_yellow}' into a temporary file '${bold_red}$backupfile.xz.dec${bold_yellow}'${nocolor}"
echo

if ! pv -W "$backupfile.xz.enc" | openssl enc -aes-256-cbc -md sha256 -salt -out "$backupfile.xz.dec" -d -pass file:/home/vlastimil/.blablablabla 2> /dev/null
then
    echo "OpenSSL failed to decrypt your encrypted file"
    exit 1
fi

echo
echo "${bold_yellow}6/6: Comparing your un-encrypted, compressed, archived, directory file '${bold_red}$backupfile.xz${bold_yellow}' with a temporary file '${bold_red}$backupfile.xz.dec${bold_yellow}'${nocolor}"
echo

if ! cmp "$backupfile.xz" "$backupfile.xz.dec"
then
    echo "Failed to compare files"
    exit 1
else
    echo "Your directory '${bold_red}$dir${nocolor}' has been successfully stored as '${bold_red}$backupfile.xz.enc${nocolor}'"
    rm "$backupfile.xz.dec"
    rm "$backupfile.xz"
    rm "$backupfile"
fi

It's a fine script. Even shellcheck doesn't find anything in it. Well done!

I only have minor comments.

You can safely omit the sha512sum -c.
It would only fail if the previous command failed.
For the rest, I cannot say it's complete paranoia.

The password file used with openssl commands is buried in the script and repeated twice.
It would be better to put that value in a variable and define at the top of the file.

A helper function would be more convenient to print the headers for the steps, instead of blocks of echos.

  before I take another step and restructure it with printf and better error reporting, structure, exit codes, etc. I would like you to have a look first

I don't see what benefits printf will bring,
and the error reporting, structure, exit codes look just fine.

I would rename bold_red and bold_yellow to be more about the purpose than presentation.
Like <em> in HTML is for emphasis,
and <strong> for strong emphasis,
And the stylesheet (in this case the actual value of the variables) define the presentation, nicely segregated from the code.


MIPS program to find the sum and square sum of 10 integers I wrote a program (hard-code) in MIPS that gets an array of 10 integers and calculates the sum and the square sum of them.
The array is {23,-2,45,67,89,12,-100,0,120,6}

.data
array: .word 23,-2,45,67,89,12,-100,0,120,6 # array = {23,-2,45,67,89,12,-100,0,120,6}
length: .word 10 # the length of the array is 10
sum: .word 0 # the sum of the integers (in array) is 0
squareSum: .word 0 # the square sum of the integers (in array) is 0
sumMessage: .asciiz "The sum of the array(sign) is: "
squareMessage: .asciiz "The sum of the squares(sign) is: "
newLine: .asciiz "\n"

# Algorithm being implemented to sum an array
# sum = 0 (use $t0 for sum)
# squarSum = 0 (use %t5 for squarSum)
# for i = 0 to length-1 do (use $t1 for i)
# sum = sum + array[i] (use $t2 for length-1)
# squareSum = squareSum + array[i]*array[i]
# end for (use $t3 for base addr. of array)

.text
main:

li $t0, 0 # load immediate 0 in register $t0 (sum)
li $t5, 0 # load immediate 0 in register $t0 (squarSum)
la $t3, array # load base addr. of array into $t3
lw $t2, length # load length in register $t2
addi $t2, $t2, -1 # $t2 = length - 1
li $t1, 0 # initialize i in $t1 to 0

loop:

bgt $t1, $t2, exit # exit loop when i > (length-1)
mul $t4, $t1, 4 # multiple i by 4 to get offset within array
add $t4, $t3, $t4 # add base addr. of array to $t4 to get addr. of array[i]
lw $t4, 0($t4) # load value of array[i] from memory into $t4
add $t0, $t0, $t4 # update sum
mul $t6, $t4, $t4 # temp register %t6
add $t5,$t5,$t6 # update squareSum
addi $t1, $t1, 1 # increment i
j loop

exit:

# print sum message
li $v0, 4
la $a0, sumMessage
syscall
# print value of sum
li $v0, 1
addi $a0,$t0,0
syscall
# print new line
li $v0, 4
la $a0, newLine
syscall
# print square sum message
li $v0, 4
la $a0, squareMessage
syscall
# print value of squareSum
li $v0, 1
addi $a0,$t5,0
syscall

Is this a good implementation?  How can I improve it?

Your program is correct. I ran it and verified the results against a C program. So, it works and the code is pretty good, too.

You did a good job of commenting. Especially, the top block where you detail the algorithm and the intended register usage. And, putting a sidebar comment on each asm line is also very good.

But, you also wanted to know how to improve it. So, a few things ...

I realize you're just starting out, but sidebar comments should talk about intent. That is, what are we doing with the algorithm, instead of just rehashing the mechanics of the asm instruction.

I tightened things up a bit. Instead of what you did (in C/pseudo code):

int i;
int val;

for (i = 0;  i <= (length - 1);  ++i) {
    val = array[i];
    ...
}

I changed this into:

int *arrptr;
int *arrend;
int val;

arrptr = array;
arrend = &array[length];
for (;  arrptr < arrend;  ++arrptr) {
    val = *arrptr;
    ...
}

I was also able to eliminate a few extra instructions by changing some of the registers being used and eliminate a extra branch instruction in the loop

Anyway, here's the cleaned up/improved code [please pardon the gratuitous style cleanup]:

    .data
array:      .word       23,-2,45,67,89,12,-100,0,120,6
arrend:

sumMessage: .asciiz     "The sum of the array(sign) is: "
squareMessage:  .asciiz "The sum of the squares(sign) is: "
newLine:    .asciiz     "\n"

    # array = {23,-2,45,67,89,12,-100,0,120,6}
    # the sum of the integers (in array) is 0
    # the square sum of the integers (in array) is 0

    # Algorithm being implemented to sum an array
    # sum = 0 (use $t0 for sum)
    # squarSum = 0 (use %t5 for squarSum)
    # for i = 0 to length-1 do (use $t1 for i)
    # sum = sum + array[i] (use $t2 for length-1)
    # squareSum = squareSum + array[i]*array[i]
    # end for (use $t3 for base addr. of array)

    # registers:
    #   t0 -- sum
    #   t5 -- squarSum
    #
    #   t3 -- pointer to current array element (e.g. arrptr)
    #   t2 -- pointer to end of array
    #
    #   t4 -- current value fetched from array
    #   t6 -- temp to hold squared value

    .text

main:
    li      $t0,0                   # sum = 0
    li      $t5,0                   # squarSum = 0

    la      $t3,array               # load base addr. of array
    la      $t2,arrend              # load address of array end
    j       test

loop:
    lw      $t4,0($t3)              # load array[i]
    addi    $t3,$t3,4               # increment array pointer

    add     $t0,$t0,$t4             # update sum
    mul     $t6,$t4,$t4             # get val * val
    add     $t5,$t5,$t6             # update squareSum

test:
    blt     $t3,$t2,loop            # more to do? if yes, loop

    # print sum message
    li      $v0,4
    la      $a0,sumMessage
    syscall

    # print value of sum
    li      $v0,1
    addi    $a0,$t0,0
    syscall

    # print new line
    li      $v0,4
    la      $a0,newLine
    syscall

    # print square sum message
    li      $v0,4
    la      $a0,squareMessage
    syscall

    # print value of squareSum
    li      $v0,1
    addi    $a0,$t5,0
    syscall

    li      $v0,10
    syscall

If you'd like to see some further tips on writing good/clean asm, based on my own personal experience, see my answer here: https://stackoverflow.com/questions/36538325/mips-linked-list/36560575#36560575

UPDATE:

  I didn't understand the "arrend" part. how does it know that $t2 = 10?

$t2 no longer has a count. It was repurposed to hold the address of the array's end address. The two loops are fundamentally different in how they iterate and terminate. This was evidenced in the C samples given above.

Your code used $t2 to hold the length/count of array, which was 10. Your loop was using an index variable [contained in $t1] to iterate through the array and stopping when the index value hit the count. It was incrementing the index value by 1 on each loop iteration.

In the modified code, $t2 holds the address of arrend. This is "one beyond" the last element of array. The loop stops when the pointer to the current array value [in $t3] hits/equals the array end. It was incrementing the pointer by 4 on each iteration.

In strictest terms, the loop doesn't use or care about the count. What it cares about is: "If I know the address of the array end, has my current address/pointer gone past it?"

Just for completeness, let's go the other way. In mars, array has address 0x10010000 and arrend has address 0x10010028. If we subtract the addresses, arrend is offset from array by 0x28 bytes, which is 40 (decimal). If we divide this by 4 [the size of a word] to get the count, we have 10


Escaping photo caption to avoid cross site script attack I am a PHP newbie and this is part of a gallery code. I recently read about SQL injection and cross site scripting attack, I am trying to avoid that by html escaping on dynamically produced data. I am not sure if I am heading in the right direction here. I created an array with URL parameters then a function for HTML escaping. Can you critique this code and let me know what I am doing wrong?

$parameters_new = array('name' => $name,'pcaption' => $caption_array[$new]);

function html_escape($input, $encoding)
{
    return htmlentities($input, ENT_QUOTES, $encoding);
}

$result_final .= '

'.$photo_caption.'

'.$photo_description.'

;

Instead of a one size fits all approach, you can filter things based on what they are.

URLs and url parameters can be encoded with urlencode()
Output inside of html tags can be filtered with strip_tags()
Other more general output that you are using with HTML attributes such as 'alt' are less predictable, and can be filtered with htmlentities()


Find unconnected nodes of a graph It is a set of edges of a graph given. An edge is given as an array of its endpoints, e.g. [:a, :b]. The array can contain an arbitrary amount of endpoints. Therefore [] and [:b, :c, :d] are also valid edges.

I want to find all islands which means that I looking for unconnected sets of nodes. Here is my Ruby solution.

e = [[:a, :b],[:c, :d],[:b, :c, :e], [:f, :g]]
n = e.flatten.uniq

def islands(edges)
  edges.reduce([]) do |islands, edge|
    matches = islands.select { |island| island.any? { |node| edge.include?(node) } }

    if !matches.empty?
      island = matches.shift
      islands.delete_if { |i| matches.include?(i) }

      island.concat(matches.flatten)
      island.concat(edge)
      island.uniq!
    else
      islands << edge
    end

    islands
  end
end

puts islands(e).inspect
# => [[:a, :b, :c, :d, :e], [:f, :g]]

Is there room for improvement?

if !matches.empty?

equals

if matches.any?

And the second is preferable, cause you don't have negation.


Boolean naming convention I have a class that stores the data related to all employees in a particular department.
In this class, I am having a list of employees and other fields.
Also, I am having a property which returns a bool, indicating whether an employee exists or not.  I am a bit confused regarding the naming of this property.

class Employee_Details
{
  #region Private Fields
  private List<Employee> employees;
  #endregion

  #region Fields
  // Gets a value indicating whether an employee exists
  public bool DoesEmployeeExists  // what should be the name of this property?
  {
    get
    {
      return this.employees.Any();
    }
  }
  #endregion
}

Is the property name DoesEmployeeExists correct, or should I use any other name?

I would use simply HasEmployees.

In case of Boolean, I always try to use hasSomething or isSomething. In Java, many libraries use this strategy (it's just a convention). And I like it, because it's so easy to read when you see:

if(annoyingEmployee.isRemovable() && !annoyingEmployee.hasBirthday()) {
  manager.fireEmployee(annoyingEmployee);
}


Clojure programming exercise for calculating change I'm learning Clojure and have written a small function to calculate change. I don't like few things about the code and would primarily like to replace recursive call with something more idiomatic. Could you please comment on the code and suggest a better (more Clojure-like) way of implementing it?

(def in-bank #{1 2 5 10})

(defn smaller-than? [compare-input metric]
  (>= compare-input metric))

(defn get-max-smaller-than [input]
  (apply max (filter (partial smaller-than? input) in-bank)))

(defn change-calc [change-requested change-given]
  (let [calculated
    (get-max-smaller-than change-requested)]
    (let [upd-change-given
      (conj change-given calculated)]
        (let [sub-request (- change-requested calculated)]
          (if (= sub-request 0)
            upd-change-given (recur sub-request upd-change-given))))))

First, there is nothing wrong with your recursive call: recur is about as idiomatic as Clojure gets.

Now, let's try to improve your code.

If you look at it, smaller-than? is a synonym for >=. It just
passes it the same arguments in the same order.

You could define it as ...

(def smaller-than? >=)

... or, better, just use >=. If you want your own name, you'd have been better to call it not-smaller-than?.

There is no need to nest lets in change-calc. One let will do.
Successive bindings    in a let are evaluated in order.
Try to choose suggestive names. I'd replace the meaningless
calculated with something like candidate.

Thus we get

(defn max-smaller-than [input]
  (apply max (filter (partial >= input) in-bank)))

(defn change-calc [change-requested change-given]
  (let [candidate (max-smaller-than change-requested)
        upd-change-given (conj change-given candidate)
        sub-request (- change-requested candidate)]
          (if (= sub-request 0)
            upd-change-given (recur sub-request upd-change-given))))

There are a couple of worries here:

We'd better deal with a zero change-requested directly than work
out it's going to be zero next time round.
The test for sub-request is for exactly zero. What happens if we
can't give exact change? (With 1 in in-bank, this will not arise.
But if it ever does, max throws an arity exception).

These revisions get us to

(defn change-calc [change-requested change-given]
  (if (pos? change-requested)
    (let [candidate (max-smaller-than change-requested)
          upd-change-given (conj change-given candidate)
          sub-request (- change-requested candidate)]
      (recur sub-request upd-change-given))
    change-given))

But this doesn't deal with the exception throwing problem. What is perhaps a better solution to this follows.

Let's look at the problem afresh.

Your code returns

its second (sequence) argument with ...
the change from the in-bank denominations conj-ed onto it, so at
the front/back if the sequence is a list/vector.

In any programming language, the second function should be distinct: especially in Clojure, where it is easy to append to a vector.

How do we do it?

For the change, we can return a count of how many coins there are of
each denomination, using a map from denomination to number. If
Clojure had a standard multiset/bag, we'd use it instead.
And let's make the denomination set an explicit argument.

If we want to make up a sum from a number of denominations, we can do the following:

(defn change [sum denoms]
  (reduce
    (fn [[ans ch :as both] d]
      (let [c (quot ch d)]
        (if (zero? c) both [(assoc ans d c) (mod ch d)])))
     [{} sum]
     (sort > denoms)))

This is more or less the algorithm you wrote, with a few wrinkles:

We sort the denominations in decreasing order.
We deal with each once and for all through a reduce.
We use quot and mod to short-circuit a loop using subtraction.

The result is a pair:

a map representing a multiset of how many (non-zero) there are of
each denomination, and
a number for how much change is left over unreconciled.

Examples:

(change 8 in-bank)
;[{1 1, 2 1, 5 1} 0]

(change 8 #{2 5 10})
;[{2 1, 5 1} 1]

(change 13 #{1 5 10})
;[{1 3, 10 1} 0]

The idea of returning the residue too arose from the algorithm, where
it is part of the working.
If 1 is a denomination, the residue is always 0.
A development might be to know how many of each coin were present to
start with.


String comparator as a lambda I am writing a Comparator for my TreeMap. It looks like this:

private static final Comparator<String> CASE_INSENSITIVE_COMPARATOR =
new Comparator<String>() {
    @Override
    public int compare(String o1, String o2) {
        int cmp = o1.toLowerCase().compareTo(o2.toLowerCase());
        if (cmp != 0) return cmp;

        return o1.compareTo(o2);
    }
};

My IDE hints me that it could be written in a lambda. I came up with this idea:

private static final Comparator<String> CASE_INSENSITIVE_COMPARATOR_LAMBDA =
(s1, s2) ->
s1.toLowerCase().compareTo(s2.toLowerCase()) == 0 ?
s1.compareTo(s2) : s1.toLowerCase().compareTo(s2.toLowerCase());

But then I am duplicating the first call s1.toLowerCase().compareTo(s2.toLowerCase(). This is doing two  String manipulation and one comparison more than the not lambda expression.

You can also chain the Comparators using thenComparing

CASE_INSENSITIVE_COMPARATOR = Comparator.comparing((Function<String, String>) String::toLowerCase)
                                        .thenComparing(o -> o);

Official Java docs on thenComparing.