Implementing NSCopying and copyWithZone I've read some resources on implementing NSCopying and copyWithZone, but things are still not very clear. I see many implementations with allocWithZone, which seems to be deprecated, and with autorelease, which seems to be unnecessary with ARC.

I was wondering if I can get away with something more simple. Something like this:

MTSound.h

#import <Foundation/Foundation.h>

@interface MTSound : NSObject <NSCopying>

@property (nonatomic) float frequency;
@property (nonatomic) float amplitude;
@property (nonatomic) float duration;

// Designated Initializer
- (instancetype)initWithFrequency:(float)freq
                        amplitude:(float)amp
                         duration:(float)dur;

+ (instancetype)soundWithFrequency:(float)freq
                         amplitude:(float)amp
                          duration:(float)dur;

@end

MTSound.m

#import "MTSound.h"

@implementation MTSound

// Initializers

- (instancetype)initWithFrequency:(float)freq amplitude:(float)amp duration:(float)dur {
    if (self = [super init]) {
        _frequency = freq;
        _amplitude = amp;
        _duration = dur;
    }
    return self;
}

// Class factory methods

+ (instancetype) soundWithFrequency:(float)freq amplitude:(float)amp duration:(float)dur {
    return [[self alloc] initWithFrequency:freq amplitude:amp duration:dur];
}

// NSCopying protocol

- (id)copyWithZone:(NSZone *)zone {
    return [[self class] soundWithFrequency:self.frequency
                                  amplitude:self.amplitude
                                   duration:self.duration];
}

@end

Is there something wrong with this approach? If so, what's the correct implementation?

Your implementation looks good to me.  From the NSCopying reference:

Your options for implementing this protocol are as follows:
Implement NSCopying using alloc and init... in classes that don’t inherit copyWithZone:.
Implement NSCopying by invoking the superclass’s copyWithZone: when NSCopying behavior is inherited. If the superclass implementation might use the NSCopyObject function, make explicit assignments to pointer instance variables for retained objects.
Implement NSCopying by retaining the original instead of creating a new copy when the class and its contents are immutable.
If a subclass inherits NSCopying from its superclass and declares additional instance variables, the subclass has to override copyWithZone: to properly handle its own instance variables, invoking the superclass’s implementation first.

So you just need to know that if you are going to have classes that inherit from your MTSound, they will need to call [super copyWithZone] first, and then initialize their own instance variables after that.
You may also want to consider the first option of simply using alloc and init.  If it is okay with Apple, then it should be alright for you to do it too, if you want.
Looking at the reference examples, it also seems perfectly fine to ignore the NSZone argument that is passed into copyWithZone.  It seems to be common practice to ignore that argument.  This Stack Overflow answer talks more about NSZone if you are interested.
I have just a couple more comments about the code.  It seems to me that these comments:
// Initializers
// Class factory methods
// NSCopying protocol

Would be more useful as pragmas, or could be removed completely.  If you do this instead:
#pragma mark - Initializer

Then you will get a bold heading in the method browser in Xcode, which can be useful for finding specific methods in a class.  This is definitely a matter of preference, but simple comments like you have are not that useful and are a bit redundant.
Is there a reason that your properties such as @property (nonatomic) float frequency; are not readonly?  From reading the code, I cannot tell if they will be modified later, or if the values will always stay the same.  If they will always stay the same, I think readonly would be appropriate.
Overall the code is very clean and easy to understand.


Reverse each number in a list I have this method which reverses a list of numbers. For example, if the given input is { 1232, 3455, 6071 }, it will return {2321, 5543, 1706}

public static List<int> ReverseIntegerList_Array(List<int> intList)
    {
        var newIntList = new List<int>();

        foreach (var numbers in intList)
        {
            var charNums = numbers.ToString();
            char[] arrNums = new char[charNums.Length];
            for (int x = 0; x < charNums.Length;x++)
            {
                arrNums[x] = charNums[charNums.Length-1-x];
            }
            newIntList.Add(Convert.ToInt32(string.Join("",arrNums)));

        }
        return newIntList;
    }

Can I reduce the space complexity further by not using an extra list object to store the result and return the result by overwriting the existing list object (defined in the method parameter)?

Found the way already.

 public static List<int> ReverseIntegerList_Array3(List<int> intList)
    {
        // var newIntList = new List<int>();
        for (int i = 0; i < intList.Count; i++)
        {

            var charNums = intList[i].ToString();
            intList[i] = 0; // Clear it.
            char[] arrNums = new char[charNums.Length];
            for (int x = 0; x < charNums.Length; x++)
            {
                arrNums[x] = charNums[charNums.Length - 1 - x];
            }
            intList[i] = Convert.ToInt32(string.Join("", arrNums));
        }

        return intList;
    }

Is this solution okay in regards to space complexity?

You can improve the efficiency by reversing the numbers as integers instead of converting to strings and back.

Modifying the list and returning it is redundant since the list is passed by reference and your changes will persist to the original list.

Simplified your code could look like this:

public static void ReverseIntegerList_Array(List<int> intList)
{
    if(intList == null)
    {
        return;
    }
    int limit = intList.Count;
    for(int i = 0; i < limit;++i)
    {
        intList[i] = ReverseNum(intList[i]);
    }
}

public static int ReverseNum(int num)
{
    int retVal = 0;
    while(num > 0)
    {
        retVal = (retVal * 10) + num % 10;
        num /= 10;
    }
    return retVal;
}


Generate sinusoid as Wave file This is a little program to show how a WAVe-File Generator could be written.
It use a Function-Object "SinFunction" to calculate the value of any point of the amplitude in time. This object and two floats are given to the constructor of the generator object. The two floats are the frequency and the duration of the created signal.

You could give it other Function-Objects to produce the function of any expression you can write in C++. Or you could give SinFunction different frequency and/or duration values.

#include <iostream>     /* Write File */
#include <fstream>
#include <string>       /* File Name Parameter */
#include <math.h>       /* sin */

using namespace std;

class SinFunction{

public:

    float operator() (float x){

        return sin(x);

    }
};

template <class Funk>
class WaveFileWriter{

private:
void longWrite(long x, ofstream &myfile)
{
  long i = x;
  int i1 = i % 256;
  i = i - i1;
  i = i / 256;
  int i2 = i % 256;
  i = i - i2;
  i = i / 256;
  int i3 = i % 256;
  i = i - i1;
  i = i / 256;
  int i4 = i;

  unsigned char a = (unsigned char)i1;
  unsigned char b = (unsigned char)i2;
  unsigned char c = (unsigned char)i3;
  unsigned char d = (unsigned char)i4;

  myfile << a;
  myfile << b;
  myfile << c;
  myfile << d;
}

void integerWrite(unsigned int x, ofstream &myfile)
{
  unsigned int i = x;
  int i1 = i % 256;
  i = i - i1;
  i = i / 256;

  unsigned char a = (unsigned char)i1;
  unsigned char b = (unsigned char)i;

  myfile << a;
  myfile << b;

}

float Frequency; // Frequency of playing Tone
float Duration ; // Duration on playing Tone
Funk Funktion;

public:
WaveFileWriter(string FileName, float Frequenz, float Duratione) : Frequency{Frequenz}, Duration{Duratione} {
  ofstream myfile;

  // Definition on File Name: "MeinTest.wav"
  myfile.open (FileName.c_str(), ios::out | ios::binary);

  // empty Char declaration
  char h = 0;
  // +++++++ Header ++++++
  // ----- WAV-Header ----
  // Chunk Name "RIFF"
  char RIFF[] = "RIFF";
  myfile << RIFF;

  // Chunk Lange
  longWrite(220000, myfile);

  // RIFF-Typ "WAVE"
  char WAVE[] = "WAVE";
  myfile << WAVE;
  // ----- FMT CHUNK ------

  // CHUNK-NAME 'FMT
  char FMT[] =  "fmt ";
  myfile << FMT;

  // CHUNK Länge 10H
  longWrite(0x10, myfile);
  // Audio-Format
  integerWrite(1, myfile);

  // Kanalzahl
  integerWrite(2, myfile);

  // Samplerate (Hz) = 44 000
  long SampelsPerSecond = 44100;
  longWrite(SampelsPerSecond, myfile);

  // Bytes pro Sekunde = 44 000
  longWrite(176000, myfile);

  // Bytes pro Sampel
  integerWrite(4, myfile);

  // Bits pro Sampel
  integerWrite(16, myfile);

  // CHUNK-Name 'data'
  char data[] = "data";
  myfile << data;
  // CHUNK Lange
  int Time = 5;
  long DataLeng = (long)SampelsPerSecond * Time;
  longWrite(DataLeng, myfile);
  // +++++ DATA ++++
  float result;
  float value;
  unsigned int f;
  for(float m = 0; m <= DataLeng; m++)
  {
    // 1. Chanel
    result = Funktion(M_PI*2*(m/SampelsPerSecond)*Frequency);
    value = (result * 32767 ) + 32767;
    f = (unsigned int)value;
    integerWrite(f, myfile);

    //2. Chanel
    result = Funktion(M_PI*2*(m/SampelsPerSecond)*Frequency);
    value = (result * 32767 ) + 32767;
    f = (unsigned int)value;
    integerWrite(f, myfile);

    // myfile << (char)value;
  }

  //  File is writen so close
  myfile.close();
}
};

int main( int argc, const char* argv[] )
{
    WaveFileWriter<SinFunction> File1{"MyTest.wav", 1000, 5};
    return 0;
}

I'm looking for ideas to refactor it, to make it perfect.

Dont't use using namespace std;

Use consistent formatting:

Your vertical whitespace and indentation are inconsistent. That makes your code a little difficult to read.

Use of access modifiers

It is unnecessary to specify private as an access modifier at the top of your class as they are private by default. However it is common to list public members and functions first because anyone (including you) reading your code is going to be more interested in the interface for the class.

Omit return 0;

main() will return on its own at the end of its execution. It is considered standard practice not to return at the end of main() and the compiler will generate the appropriate return as needed.

Let RAII do its job

{
ofstream myfile;

// Definition on File Name: "MeinTest.wav"
myfile.open (FileName.c_str(), ios::out | ios::binary);

… // other code
//  File is writen so close
myfile.close();
}

The very next line after you close your file explicitly the scope ends. Your file will close itself on going out of scope and perform all necessary operations (like flushing and memory cleanup) A huge part of writing quality C++ classes is proper RAII cleanup so that you and I don't need to clean up after ourselves. Let your objects go out of scope instead of doing it explicity.

Only write meaningful comments

Most of your comments (like those in the previous example) only serve to state what is already clearly stated by the code itself. As a rule of thumb you should always assume that anyone reading your code is capable of reading code. myFile.close() should be readily apparent as to what it does. Only comment when something isn't easy to understand. (also consider refactoring or renaming when you find the need for a comment.)

Don't use All-cap variable names

char RIFF[] = "RIFF";
char WAVE[] = "WAVE";

All cap variables are conventionally reserved for macros. prefer instead:

char Riff[] = "RIFF";
char Wave[] = "WAVE";

or

char riff[] = "RIFF";
char wave[] = "WAVE";

Incidentally I am a bit uncertain about the current use case but I am unsure why you are using C-style char array[] (and not char* array[]) or a C++ string? is there a reason you can't use an std::string?

Use C++ casts

unsigned char a = (unsigned char)i1;
unsigned char b = (unsigned char)i2;
unsigned char c = (unsigned char)i3;
unsigned char d = (unsigned char)i4;

C++ static_cast<> is safer than C-style casts as it is checked at compile time. Also you should use C++ conventions when writing C++ code.

Concatenate your operations

long i = x;
int i1 = i % 256;
i = i - i1;
i = i / 256;
int i2 = i % 256;
i = i - i2;
i = i / 256;
int i3 = i % 256;
i = i - i1;
i = i / 256;
int i4 = i;

unsigned char a = (unsigned char)i1;
unsigned char b = (unsigned char)i2;
unsigned char c = (unsigned char)i3;
unsigned char d = (unsigned char)i4;

can be re-written:

int x1 = x % 256;
x = (x - x1) / 256;
int x2 = x % 256;
x = (x - x2) / 256;
int x3 = x % 256;
x = (x - x1) / 256; // did you mean to use x3 here? (well i3 in your code)
int x4 = x;

unsigned char a = static_cast<unsigned char>(x1);
unsigned char b = static_cast<unsigned char>(x2);
unsigned char c = static_cast<unsigned char>(x3);
unsigned char d = static_cast<unsigned char>(x4);

notice it was also unnecessary to copy x into a second variable. you can use it directly (it's already a copy)

Omit unused headers

You aren't using <iostream>. to that end you can omit it.

Compare values of the same type

for(float m = 0; m <= DataLeng; m++)

That line threw me off for a moment. I've never seen a float as a comparison within a for loop. Especially since DataLeng is an int. Your compiler should have warned you about this. To that end. . . :

Use max compiler warnings

Make sure you have max compiler warnings on. Consider warnings as error too. This will catch a lot of issues for you and help you clean your code before you even get to us. use -W4 on VS or -Wall on gcc/clang and -Werror. (If you use a different compiler you will have to look up how to better utilize warnings.)

Prefer prefix over postfix


Note class for managing information on various musical notes I just finished (re)reading Clean Code, and one of the ideas I learned was the SRP. However, I am not quite sure on what defines a "Single Responsibility." In the piece of code below, I have a Note class which I think may be doing too many things; it stores pitch and length, and converts these into a Names enum and a Lengths enum. Is this more than one responsibility, and if so, how should I define the classes instead?  Other tips are welcome as well).

public class Note {

    public enum Lengths {
        DOUBLE_WHOLE_NOTE(0.5f),
        ...
        SIXTYFOURTH_NOTE(64);

        private float length;

        Lengths(float length) {

            this.length = length;
        }

        public float getLength() {

            return length;
        }

        private static final Map<Float, Lengths> lookup = new HashMap<Float, Lengths>();
        static {
            for (Lengths length : Lengths.values())
                lookup.put(length.getLength(), length);
        }

        public static Lengths get(float length) {

            return lookup.get(length);
        }

    }

    public enum Names {
        C(0),
        ...
        REST(-1);

        private final int number;

        Names(int number) {

            this.number = number;
        }

        public int getValue() {

            return number;
        }

        private static final Map<Integer, Names> lookup = new HashMap<Integer, Names>();
        static {
            for (Names name : Names.values())
                lookup.put(name.getValue(), name);
        }

        public static Names get(Integer value) {

            return lookup.get(value);
        }

    }

    private int pitch;
    private float length;

    public int getPitch() {

        return pitch;
    }

    public void setPitch(int pitch) {

        this.pitch = pitch;
    }

    public float getLength() {

        return length;
    }

    public void setLength(float length) {

        this.length = length;
    }

    private Note(int pitch, float length) {

        this.length = length;
        this.pitch = pitch;
    }

    public int getOctave() {

        return (pitch / 12) - 5;
    }

    public Note.Names getNameOfNote() {

        return Names.get(this.getPitch() % 12);
    }

    public Note.Lengths getLengthOfNote() {

        return Lengths.get(this.getLength());
    }

}

Although you put the three classes into one file, they are already three classes. So you already split the responsibility. Translating a float to a length is one "responsibility" and translating the pitch to a name is another. Seen from this angle everything is fine.

SRP is a heuristic for good OO code. You won't go straight to hell if your class does two things. But if your class does 20 things you should consider a refactoring.

If you have the feeling that your code is not readable or your file is too long, just extract the inner enum to standalone enum. In most cases it is no difference if you have an inner class/enum or a real one, so you should prefer the more readable solution.

In addition to that, I would store the length as the enum at the note and translate it once in the setter (reused in the constructor) instead doing it every time in the getter. I'm also in doubt if I would offer an interface with a integer length getter and the enum length getter. Usually you want to force the caller to use the enum object, I guess.

You should try to use this and the getters in a consistent way.

Have you considered all the nasty pitfalls in using the float type? If you don't want to do any math I would prefer String, otherwise you might want to think about using BigDecimal.


Django BaseController A working example of mine, trying to streamlining my Django views, with absolute inspiration from Laravel:

class BaseController(View):
    template_404 = "404.html"

    def __call__(self, *args, **kwargs):
        return self.view(*args, **kwargs)

    def view(self, request, slug=None):
        method = request.method.lower()
        if hasattr(self, method):
            return getattr(self, method)(request, slug)
        return self.forbidden(request)

    def get(self, request, slug):
        if slug is not None:
            return getattr(self, "show")(request, slug) if hasattr(self, "index") else self.forbidden(request)
        return getattr(self, "index")(request) if hasattr(self, "index") else self.forbidden(request)

    def post(self, request, slug):
        return getattr(self, "store")(request) if hasattr(self, "store") else self.forbidden(request)

    @staticmethod
    def forbidden(request):
        return HttpResponseForbidden("Method not allowed")

    def page_not_found(self, request):
        return render(request, self.template_404)

Inherited by:

class EulerController(BaseController):
    template_name_index = "euler_index.html"
    template_name_show = "euler_show.html"

    def index(self, request):
        euler_posts = EulerPost.objects.all()
        return render(request, self.template_name_index, {"euler_posts": euler_posts})

    def show(self, request, slug):
        euler_post = EulerPost.objects.filter(slug=slug).first()
        return render(request, self.template_name_show, {"euler_post": euler_post})

and called by:

urlpatterns = [
    url(r'^$', EulerController(), name='main'),
    url(r'^(?P<slug>[\w-]+)/$', EulerController(), name='resource'),
]

and the mock model:

class EulerPost(model):
    slug = models.CharField(default=None, max_length=200, blank=False, unique=True)

I left Django for PHP-Laravel, long ago believing myself to be an to unstructured for the freedom of Django.  But now I feel I am on the right track.

It is usually better to ask forgiveness than permission and explicit is better than implicit. So I would change your getattr..if hasattr to a try..except, which is a lot easier to understand:

def post(self, request, slug):
    try:
        return self.store(request)
    except AttributeError:
        raise HttpResponseForbidden("Method not allowed")

And similar for get and view:

def get(self, request, slug):
    try:
        if slug is not None:
            return self.show(request, slug)
        return self.index(request)
    except AttributeError:
        raise HttpResponseForbidden("Method not allowed")

def view(self, request, slug=None):
    try:
        return getattr(self, request.method.lower())(request, slug)
    except AttributeError:
        raise HttpResponseForbidden("Method not allowed")


Read multidimensional array from file in C I want to read multidimensional double array from file into a single pointer variable. The number of rows and columns are not known and need to be determined when reading the file in order to allocate memory for the array. I wrote the following code and it works fine with me. I want to know if someone has more efficient solution for such case:

//Initialize variables and open the file
size_t count=1000;
char *line = malloc(1000);
FILE *file= fopen("g.txt", "r");
if(file==NULL)
{
    printf("Error in Opening file");
    return EXIT_FAILURE;
}
double *data=(double*) malloc(1000*sizeof(double));
if(data==NULL)
{
    printf("Error in allocating memory");
    return EXIT_FAILURE;
}

// Read number of columns and number of rows
getline(&line, &count, file);
int read=-1,cur=0,columCount=0;
while(sscanf(line+cur, "%lf%n", &data[columCount],&read)==1)
    {cur+=read;columCount=columCount+1;}

int rowCount=1;
while(getline(&line, &count, file)!=-1) {rowCount=rowCount+1;}

rewind(file);

// Reinitialize array using the number of rows and number of columns
free(data);
data=(double*) malloc(columCount*rowCount*sizeof(double));
if(data==NULL)
{
        printf("Error in allocating memory");
        return EXIT_FAILURE;
}

// Read file and store values
int i=0;
while(getline(&line, &count, file)!=-1)
{
    read=-1,cur=0;
    while(sscanf(line+cur, "%lf%n", &data[i],&read)==1)
    {cur+=read;i=i+1;}
}

fclose(file);
return EXIT_SUCCESS;

You have written everything in one function (main probably) but it is better
split into its components.  Essentially what you want to do is:

open input file
get column count
get line count
rewind file
allocate memory for the array
read array data

Some of these stages could and probably should be in separate functions.

Taking it from the top, you open the input file, with the file name
hard-coded.  A better approach is to get the filename from the command line -
argv[1].  You have checked for failure to open the file, which is good.
The normal error handling is to use perror and the name of the failed file:

FILE *f = fopen(argv[1], "r");
if (!f) {
    perror(argv[1]);
    return EXIT_FAILURE;
}

You then allocate buffer data (again the error handling should use
perror).  You then use this buffer in a read loop to count the entries in
each line, but you really don't need to save the values in the buffer at all
at this stage - you discard them anyway. Your read loop could equally be:

while (sscanf(line+cur, "%*lf%n", &read) == 1) {
    cur += read;
    columCount++;
}

Notice that the format string uses a '*' to suppress assignment of the
floating point value read.  Note also the improved formatting and the use of
++ in columCount++.  Note also that you don't check for failure in reading
the first line.

Edit: Abbas pointed out that adding the '*' to suppress assignment means that we must check for 0 returned from sscanf, not 1. But sscanf also returns 0 when it fails to read anything (in this case the read count in read would also contain 0). Hence a bad character in the line will cause the loop to hang. So we must either not suppress the assignment:

double d;
while (sscanf(line+cur, "%lf%n", &d, &read) == 1) {
    cur += read;
    columCount++;
}

or we must check read.

while ((sscanf(line+cur, "%*lf%n", &read) == 0) && (read != 0)) {
    cur += read;
    columCount++;
}

I think I would go for the first.  End of edit.

You then count the lines by looping through them.  Note that you use a fixed
size buffer line, allocated at the beginning, to read each line.  To be
completely flexible, you should handle lines of arbitrary length.  Your lines
are fixed at 1000 chars and anything beyond that breaks the code.  Since you
are using getline (which is non-standard, but useful) I would make use of its
ability to allocate a line for you by passing a zero size.

char *line = NULL;
size_t count = 0;            // EDIT: Abbas pointed out we need to add count=0
getline(&line, &count, file);
// use the line
free(line);

In this, line is just a pointer. getline allocates a buffer large enough
to hold the line of text and places a pointer to the buffer in line.  You
must free that line later.

So now you have the column and row count.  Those two stages (reading the two
sizes) would be better extracted into a separate function.

You now rewind the file and allocate the real array.  As stated above, this
should be the first allocation of the program.  Following this you read the
data into the array.  This might be better split into a separate function.
The read loop can be simpler than yours - you
don't need to get a line and then scan it - just scan it directly:

int ndata = 0;
while (scanf("%lg", &data[ndata++]) == 1) {
    // nothing
}
ndata -= columCount * rowCount;
if (ndata != 0) {
    printf("%s data values in input\n", ndata > 0 ? "Too few" : "Too many");
    return EXIT_FAILURE;
}

And remember to check that the number of doubles read matches what you were
expecting.

Note that scanf and its associates do not react well to malformed input.  There are other ways of reading text but in the simple case (of properly formed data) they are more complicated.  Someone else might give you some examples/alternatives.

Finally note that your formatting is inconsistent, as if bits have been cut
and pasted from elsewhere.  Copying code from your previous work is not
necessarily bad, but make sure you make the whole thing looks as if it belongs
together.


QT - Class For Connecting to Mqtt Broker With QMqttClient Task:

Class for connecting to a mqtt broker using QMqttClient and getting notified on connect and disconnect.

Context:

I'm trying to write modern C++ (standard 2011 and up) code - using smart pointers. I have only created small projects for learning C++. I'm not very confident if I'm using those pointers correctly.

The implementation:

The implementation is in the client.h and client.cpp header file. The first code snippet is the client.h one.

#ifndef CLIENT_H
#define CLIENT_H

#include <QString>
#include <QObject>
#include <QtMqtt/QtMqtt>
#include <memory>
#include <iostream>

struct MqttConfig {
    MqttConfig() {
        hostname = "localhost";
        port = 1883;
    }

    QString hostname;
    quint16 port;
};

struct SubscriptionConfig
{
    SubscriptionConfig() = delete;
    SubscriptionConfig(QString t, quint16 q) :
        topic(t), qos(q)
    {}

    QString topic;
    quint16 qos; // quality of service
};

class Client : public QObject
{
    Q_OBJECT

private:
    std::unique_ptr<QMqttClient> mqtt;
    std::unique_ptr<MqttConfig> c;

public:
    Client();
    void connectToSignals();
    void configureClient(std::unique_ptr<MqttConfig>&);
    void connect();

public slots:
    void onConnected()
    { std::cout << "Connected to mqtt broker " << std::endl; }
    void onDisconnected()
    { std::cout << "Disconnected from mqtt broker" << std::endl; }
};

#endif // CLIENT_H

Am I violating the DRY principle (don't repeat yourself) in the method connectToSignals ? And how could I avoid this?

#include "client.h"

Client::Client()
{
    mqtt = std::make_unique<QMqttClient>();
    c = std::make_unique<MqttConfig>();
    configureClient(c);
    connectToSignals();
    connect();
}

void Client::connectToSignals()
{
    QObject::connect(Client::mqtt.get(), &QMqttClient::connected, this,
                     &Client::onConnected);
    QObject::connect(Client::mqtt.get(), &QMqttClient::disconnected, this,
                     &Client::onDisconnected);
}

void Client::configureClient(std::unique_ptr<MqttConfig>& c)
{
    mqtt->setHostname(c->hostname);
    mqtt->setPort(c->port);
}

void Client::connect()
{
    mqtt->connectToHost();
}

Qt-ness

My QT is a little bit rusty, but you're probably not doing the right thing here. First QObject have their own memory management, usually each QObject constructor takes a parent, this parent takes ownership of the created object. As the setParent() function is always available, interfering with the memory management via std::unique_ptr or std::shared_ptr is usually not a good idea.

Additionally you are containing QMQTTClient for what looks like just configuration reasons, there really doesn't seem to be any need to wrap it in another layer, you might as well just replace your client with QMQTTClient and connect to it. If you want to wrap configuration, a free function will do with much less overhead.

Stack-vs-Heap

Both the QMQTTClient and the MQTTConfig could as well be stack allocated, making everything easier. Passing MQTTConfig by reference will work well enough.

MQTTConfig

The constructor should use initializer lists and not the function body for member variable initialization.

MqttConfig() : hostname("localhost"), port(1883) {}

I'd probably prefer default values for this,

MqttConfig(QString h = "localhost", qint16 p = 1883  ) : hostname(h), port(p) {}

SubscriptionConfig

The default constructor doesn't need to be deleted if you are defining a custom constructor


Python3 Stack implementation with List built-in - Round Two Previous code is available at Python3 Stack implementation with List built-in, this is a second-round review after I made alterations.

Once again, let's review my Stack implementation, with built-in lists, and with review notes from the prior review implemented.

Any changes and suggestions are welcome!

class Stack:
    def __init__(self):
        self._stack = []

    def __len__(self):
        return len(self._stack)

    def pop(self):
        if len(self) == 0:
            raise IndexError("You cannot pop from an empty stack.")

        return self._stack.pop()

    def peek(self):
        if len(self) == 0:
            raise IndexError("You cannot peek into an empty stack.")

        return self._stack[len(self) - 1]

    def push(self, item):
        self._stack.append(item)

    def __iter__(self):
        return iter(self._stack)

If you don't care if the error is textually correct, then you can remove the if in pop and peek.
You don't need to find the length of the stack to find the last element. You can just use self._stack[-1].

Creating:

class Stack:
    def __init__(self):
        self._stack = []

    def __len__(self):
        return len(self._stack)

    def pop(self):
        return self._stack.pop()

    def peek(self):
        return self._stack[-1]

    def push(self, item):
        self._stack.append(item)

    def __iter__(self):
        return iter(self._stack)

From this, it begs the question why not inherit from list? Which would make the code:

class Stack(list):
    def peek(self):
        return self[-1]

    def push(self, item):
        self.append(item)

If you're likely to switch to another form of Stack, then you may want to keep with this. However, for the most part I'd recommend not using this class. As the only benefit it has over list is that it's likely to work with more Stack interfaces.


WorkGroup Data Service with JSON / Web based API I'm wondering if this code can be made more clear and fluent:

using System;
using System.Threading.Tasks;
using Windows.Data.Json;
using System.Collections.ObjectModel;
using System.Net.Http;
using System.Collections.Generic;
using Newtonsoft.Json;
using System.Linq;
using eTest.Space.Log;
using eTest.Space.Interface;
using eTest.Objects;

namespace eTest.Space.Services
{

    public class WorkGroupDataService : IWorkgroupDataService
    {

        public string WebApiUri
        {
            get { return Constants.ServerAddress; }
        }

        private ObservableCollection<IWorkgroup> Groups;// = new ObservableCollection<IWorkgroup>();

        public async Task<ObservableCollection<IWorkgroup>> GetAllWorkGroups()
        {
            Groups = new ObservableCollection<IWorkgroup>();
            //if (Groups.Count == 0)
            {
                Uri dataUri = new Uri(WebApiUri + "api/workgroup/");

                using (var client = new Windows.Web.Http.HttpClient())
                {
                    var response = await client.GetAsync(dataUri).AsTask(); ;

                    string jsonText = response.Content.ReadAsStringAsync().GetResults();
                    JsonArray jsonArray = JsonArray.Parse(jsonText);

                    foreach (JsonValue groupValue in jsonArray)
                    {
                        JsonObject groupObject = groupValue.GetObject();
                        IWorkgroup group = new Workgroup();
                        group.GroupState = (int)groupObject["GroupState"].GetNumber();
                        group.WorkgroupName = groupObject["WorkgroupName"].GetString();
                        group.WorkgroupSecretary = groupObject["WorkgroupProba"].GetString();
                        group.ObjectID = groupObject["ObjectID"].GetString();
                        Uri dataUri1 = new Uri(WebApiUri + "api/etest/" + group.ObjectID);
                        using (var client1 = new Windows.Web.Http.HttpClient())
                        {
                            var response1 = await client.GetAsync(dataUri1).AsTask();
                            if (response1.StatusCode == Windows.Web.Http.HttpStatusCode.Ok)
                            {
                                string jsonText1 = response1.Content.ReadAsStringAsync().GetResults();
                                List<TestSession> Test = JsonConvert.DeserializeObject<List<TestSession>>(jsonText1);
                                group.Test = Test;
                            }
                        }
                        if (group.Test.Count > 0)
                        {
                            Groups.Add(group);
                        }

                    }
                }
            }
            Groups = new ObservableCollection<IWorkgroup>(Groups.OrderByDescending((x => x.Test.Count)));
            return Groups;
        }
    }
}

Naming Conventions

Private instance fields should be lowercase:

private ObservableCollection<IWorkgroup> groups;

Local variables should also be lowercase:

List<TestSession> test = JsonConvert.DeserializeObject<List<TestSession>>(jsonText1);

Also, where you are doing the same thing with the clients and URIs, you are adding numbers to the second of those variables, e.g. client1. Try to think of something more descriptive, those two clients are serving two different purposes so try something like purposeClient where purpose is replaced by a word for it's purpose. At this point, extracting the functionality out into separate methods is probably a good idea.

Improvements

If you're going to use an async method, then you should await it:

var jsonText = await response.Content.ReadAsStringAsync().GetResults();

Structure

Similarly, given that this variable doesn't look like it needs to be shared amongst methods (unless you haven't included the rest of your class), I'm not really sure why you've made it an instance field and can quite comfortably be declared inside your GetAllWorkGroups() method.

You seem to assign a value to Groups twice in that method, you don't need to do the second one:

groups = groups.OrderByDescending(x => x.Test.Count);

You also seems to double wrap the OrderByDescending clause in brackets which was unnecessary

You seem to be switching between using var and, for example string. A lot of people do switch, using var where the type inferred is obvious and using the type when it is not immediately obvious to make it more readable (this is what I do), but a lot of people will exclusively use var. In terms of style, I'd definitely go one way or the other because currently your switching just seems messy.

I find that object initializers make the code more readable so here:

IWorkgroup group = new Workgroup();
group.GroupState = (int)groupObject["GroupState"].GetNumber();
group.WorkgroupName = groupObject["WorkgroupName"].GetString();
group.WorkgroupSecretary = groupObject["WorkgroupProba"].GetString();
group.ObjectID = groupObject["ObjectID"].GetString();

Can be:

IWorkgroup group = new Workgroup
{
    GroupState = (int)groupObject["GroupState"].GetNumber(),
    WorkgroupName = groupObject["WorkgroupName"].GetString(),
    WorkgroupSecretary = groupObject["WorkgroupProba"].GetString(),
    ObjectID = groupObject["ObjectID"].GetString()
};

After this point, you then start a new using, but from the code it seems that you are in fact finished using your previous client so you can move it around to be more like this:

public async Task<ObservableCollection<IWorkgroup>> GetAllWorkGroups()
{
    var groups = new ObservableCollection<IWorkgroup>();
    JsonArray jsonArray;

    using (var client = new Windows.Web.Http.HttpClient())
    {
        var dataUri = new Uri(WebApiUri + "api/workgroup/");
        var response = await client.GetAsync(dataUri).AsTask();

        var jsonText = await response.Content.ReadAsStringAsync().GetResults();
        jsonArray = JsonArray.Parse(jsonText);
    }

    foreach (var groupValue in jsonArray)
    {
        var groupObject = groupValue.GetObject();
        IWorkgroup group = new Workgroup
        {
            GroupState = (int)groupObject["GroupState"].GetNumber(),
            WorkgroupName = groupObject["WorkgroupName"].GetString(),
            WorkgroupSecretary = groupObject["WorkgroupProba"].GetString(),
            ObjectID = groupObject["ObjectID"].GetString()
        };

        var dataUri = new Uri(WebApiUri + "api/etest/" + group.ObjectID);
        using (var client = new Windows.Web.Http.HttpClient())
        {
            var response = await client.GetAsync(dataUri).AsTask();
            if (response.StatusCode == Windows.Web.Http.HttpStatusCode.Ok)
            {
                var jsonText = await response.Content.ReadAsStringAsync().GetResults();
                var test = JsonConvert.DeserializeObject<List<TestSession>>(jsonText);
                group.Test = test;
            }
        }

        if (group.Test.Count > 0)
        {
            groups.Add(group);
        }
    }

    return groups.OrderByDescending(x => x.Test.Count);
}


Sieve of Eratosthenes implementation is running slowly I have written this code to find prime numbers, and it works well, but the calculation speeds are incredibly slow.  Am I doing this wrong? I know that I might be really doing this the wrong way, but please help me!

using System;
using System.Collections.Generic;

namespace Primenumbers
{
class MainClass
{
    public static void Main (string[] args)
    {

        List<int> NoPrime = new List<int>();

        for(int x = 2; x < 10000;x++)
        {
            for(int y = x * 2;y < 10000;y = y + x)
            {

            if(!NoPrime.Contains(y))
                {
                    NoPrime.Add(y);
                }

            }

        }

        for(int z = 2; z < 10000;z++)
        {
            if(!NoPrime.Contains(z))
            {
                Console.WriteLine(z);
            }
        }

    }
}
}

There is some room for improvement in your code, and I will try to explain that in
several steps. But please note that I am not a C# person, so my review refers only
to the algorithm and performance itself and not to the style or any language
specifics. And most probably I am making a lot of C# errors here.

The following tests were done on a MacBook Pro using
Mono.

To show the performance improvement gained in each step, I have rewritten your code a bit:

The prime numbers are not printed because printing many lines also takes time. Instead, only the number of primes is counted.
The time for the computation is taken and printed with StopWatch.
Instead of a fixed upper bound (10,000 in your code) the limit is read from
the command line arguments. I used an upper bound of 50,000.

So your code is equivalent to this:

using System;
using System.Collections.Generic;
using System.Diagnostics; // (for Stopwatch)

namespace Primenumbers
{
    class MainClass
    {
        public static void Main (string[] args)
        {
            int maxPrime = int.Parse(args[0]);
            Stopwatch stopWatch = Stopwatch.StartNew();
            int numPrimes = sieve(maxPrime);
            stopWatch.Stop();
            Console.WriteLine("{0} primes in the range 2..{1}, time taken: {2}ms", numPrimes, maxPrime, stopWatch.Elapsed.TotalMilliseconds);
        }

        static int sieve(int maxPrime)
        {
            List<int> NoPrime = new List<int>();
            for (int x = 2; x <= maxPrime; x++)
            {
                for (int y = x * 2; y <= maxPrime; y = y + x)
                {
                    if (!NoPrime.Contains(y))
                    {
                        NoPrime.Add(y);
                    }
                }
            }

            int numPrimes = 0;
            for (int z = 2; z <= maxPrime; z++)
            {
                if (!NoPrime.Contains(z))
                {
                    numPrimes++;
                }
            }
            return numPrimes;
        }
    }
}

and the output is:

5133 primes in the range 2..50000, time taken: 5744.7922ms

Step 1: Jeroen Vannevel suggested to use HashSet instead of List,
and indeed this reduces the runtime considerably:

5133 primes in the range 2..50000, time taken: 17.7239ms

But an array of booleans is much faster because it allows direct access to all elements
without any lookup. Note that this does need not much more memory. The number of
primes below a number N is approximately N/log(N), so "most" numbers are
not primes. For example, for our upper limit of 50,000 we need an array with
50,000 elements instead of a hash set with 44,867 = 50,000-5,133 elements.

With an array of booleans the sieve method would be (thanks to @abligh for suggesting
a better variable name):

static int sieve(int maxPrime)
{
    bool[] isComposite = new bool[maxPrime + 1];
    for (int x = 2; x <= maxPrime; x++)
    {
        for (int y = x * 2; y <= maxPrime; y = y + x)
        {
            if (!isComposite[y])
            {
                isComposite[y] = true;
            }
        }
    }

    // Count primes ...
    return numPrimes;
}

and now the output is

5133 primes in the range 2..50000, time taken: 1.1336ms

Step 2: If x in the for (int x = ...) loop has already marked as "not a
prime" then all the multiples of x are also not primes, which means that
the inner y-loop needs not to be executed at all. On the other hand, the
multiples y can simply be marked without checking them before:

static int sieve(int maxPrime)
{
    bool[] isComposite = new bool[maxPrime + 1];
    for (int x = 2; x <= maxPrime; x++)
    {
        if (!isComposite[x])
        {
            for (int y = x * 2; y <= maxPrime; y = y + x)
            {
                isComposite[y] = true;
            }
        }
    }

    // Count primes ...
    return numPrimes;
}

Output:

5133 primes in the range 2..50000, time taken: 0.6805ms

Step 3: If a prime x has been found, all lower primes have already been
tested and their multiples marked. Therefore y in the inner loop can start
as x * x instead of x * 2. As a consequence, x needs only to run
from 2 to sqrt(maxPrime):

static int sieve(int maxPrime)
{
    bool[] isComposite = new bool[maxPrime + 1];
    for (int x = 2; x * x <= maxPrime; x++)
    {
        if (!isComposite[x])
        {
            for (int y = x * x; y <= maxPrime; y = y + x)
            {
                isComposite[y] = true;
            }
        }
    }

    // Count primes ...
    return numPrimes;
}

Output:

5133 primes in the range 2..50000, time taken: 0.4782ms

More improvements are possible, e.g. test only 2 and then every odd number,
but I hope this is sufficient for a start.

Finally, here are my measurements for an upper limit of 2,000,000:

Your method: Don't know, did not wait so long
Using a hash set: 2601.3797ms
Using a bool array: 55.2455ms
After applying "step 2": 19.2888ms
After applying "step 3": 11.6878ms


Building a UI to fetch list of features and edit them I am building a UI which fetches a list of features based on an ID and then the user can edit the list of features and save it. The script also build the UI based on the data structure currently hard coded in.

Was hoping you guys review it and let me know how I can improve it

    "use strict";

var activeFeatures = [
      //hardcoded structure
      {
        feature: 'feautreName',
        variants: {
          'featureVariant': 'percentage'
        }
      }
    ],
    //container there the ui will be build
    list = document.querySelector('#features');

// builds the vairants per variant
function injectVariants(variants) {
  var variantHtml = '';

  Object.keys(variants).forEach(function (variant) {
    var percentage = variants[variant] || 0;
    variantHtml += '
' + variant + '<input type="text" id="' + variant + '" class="percentage-active" value="' + percentage + '">
'
  })

  return variantHtml;
}

// builds the list of features
function buildFeaturesList(featureList) {
  let featureList = featureList || activeFeatures
  list.innerHTML = '';

  featureList.forEach(item => {
    list.innerHTML += '
 ' + item.feature + '
' + injectVariants(item.variants) + '

';
  })

}

// gets document based on Id and click checked if feature is checked
function getDoumentWithFeautres() {
  var r = new XMLHttpRequest(),
      documentId = document.getElementById("documentFeatures").value;

  if (documentId !== '') {
    r.open('GET', '/id/' + documentId, true);
    r.onreadystatechange = function () {
      if (r.readyState != 4 || r.status != 200) return;
      let currentFeatures = JSON.parse(r.responseText).features || [];

      buildFeaturesList(currentFeatures);

      currentFeatures.forEach(item => {
          document.getElementById(item.feature).querySelector('.checkbox').setAttribute('checked', true);
      });
    };
    r.send();
  }
}

// each feautre has a checkbox for user to select a feature
// this is for toggles
const toggleCheckBox = (target) => {
  if (target.getAttribute('checked') === "true") {
    target.setAttribute('checked', false);
  } else {
    target.setAttribute('checked', true);
  }
}

//build the payload before posting
function getActiveFeatures() {
  var listOfFeaturesElements = [].slice.call(document.querySelectorAll(".checkbox"));
  var payload = {};

  Payload.id = document.getElementById("documentFeatures").value;
  payload.features = [];

  listOfFeaturesElements.forEach(function (element) {
    var featureObject;

    if (element.getAttribute("checked") === 'true') {

      featureObject = (activeFeatures.filter(function (obj) {
          return obj.feature === element.getAttribute('data-feature')
      }));

      Object.keys(featureObject[0].variants).map(function (item) {
          featureObject[0].variants[item] = document.getElementById(item).value
      });

      payload.features.push(featureObject[0]);
    }
  });

  return payload;
}

// post updated features on save
function updateDocument(target) {
  var payload = getActiveFeatures(),
      r = new XMLHttpRequest();

  r.open("POST", "/journeys", true);
  r.setRequestHeader("Content-Type", "application/json;charset=UTF-8");
  r.onreadystatechange = function () {
      if (r.readyState != 4 || r.status != 200) return;
  };
  r.send(JSON.stringify(payload));
}

document.body.addEventListener('click', function (e) {
  switch (e.target.classList[0]) {
    case 'search':
      getDoumentWithFeautres()
      break;
    case 'checkbox':
      if (journey) toggleCheckBox();
      break;
    case 'save':
      if (journey) updateDocument(e.target);
      break;
    default: break;
  }
});

buildFeaturesList();

Syntax and Compatibility

Most browsers will balk at the redefinition of a function parameter using the let keyword here:

// builds the list of features
function buildFeaturesList(featureList) {
    let featureList = featureList || activeFeatures

Chrome: Uncaught SyntaxError: Identifier 'featureList' has already been declared
Firefox: SyntaxError: redeclaration of formal parameter featureList
IE11: Let/Const redeclaration

This can be corrected by replacing let with var in this instance. Note that the primary difference between let and var is that let is block-scoped, while var is function-scoped. Since the block containing this variable declaration is a function anyway, you don't gain any advantage by using let.

IE11 also does not yet support arrow (=>) syntax for lambda functions, but if you don't have to support such users you can ignore that problem.

Avoid modifying innerHTML directly

Direct modification of an element's innerHTML property should be avoided whenever possible. It has performance drawbacks (forcing the browser to re-evaluate the DOM with each modification) and it can create memory leaks (when code retains references to elements that have been destroyed by overwriting their HTML).

Instead, consider these approaches:

Clearing innerHTML

To clear the innerHTML without creating memory leaks, instead of features.innerHTML = ''; you can use a loop to remove all child nodes:

while(features.childNodes.length > 0){
    features.removeChild(features.childNodes[features.childNodes.length - 1]);
}

A for loop can be more efficient since you can avoid accessing the childNodes.length property multiple times.

for(let i = features.childNodes.length-1; i >= 0; i--){
    features.removeChild(features.childNodes[i]);
}

Appending to innerHTML

To append to innerHTML without forcing the DOM to reload with every change, you can use element.insertAdjacentHTML().

For example, instead of list.innerHTML += stringValue use list.insertAdjacentHTML("beforeend",stringValue):

list.insertAdjacentHTML("beforeend",
    '
 '
    + item.feature + '
'
    + injectVariants(item.variants) + '

'
);

Note that you might also want to consider element.appendChild() which lets you append any HTML element by its reference, such as the reference obtained when you create an element using document.createElement(). That approach is more versatile (you can retain references to the elements in code and attach event handlers to the elements programmatically) but will be slightly slower than element.insertAdjacentHTML().


LeetCode: Binary Tree Level Order Traversal C# Please review for performance. and also naming convention

https://leetcode.com/problems/binary-tree-level-order-traversal/

  Given a binary tree, return the level order traversal of its nodes'
  values. (ie, from left to right, level by level).

  For example:

Given binary tree [3,9,20,null,null,15,7],
    3
   / \
  9  20
    /  \
   15   7

return its level order traversal as:
[
  [3],
  [9,20],
  [15,7]
]

using System.Collections.Generic;
using System.Linq;
using GraphsQuestions;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace TreeQuestions
{
    /// <summary>
    /// https://leetcode.com/problems/binary-tree-level-order-traversal/
    /// </summary>
    [TestClass]
    public class BinaryTreeLevelOrderTraversalTest
    {

        //      3
        //     / \
        //    9  20
        //   /  \
        //  15   7
        [TestMethod]
        public void TreeTest()
        {
            TreeNode root = new TreeNode(3);
            root.left = new TreeNode(9);
            root.right = new TreeNode(20);
            root.left.left = new TreeNode(15);
            root.left.right = new TreeNode(7);
            IList<IList<int>> res = BinaryTreeLevelOrderTraversalClass.LevelOrder(root);
            CollectionAssert.AreEqual(res[0].ToList(), new List<int>{3});
            CollectionAssert.AreEqual(res[1].ToList(), new List<int>{9,20});
            CollectionAssert.AreEqual(res[2].ToList(), new List<int>{15,7});
        }

        [TestMethod]
        public void OneNodeTreeTest()
        {
            TreeNode root = new TreeNode(3);
            IList<IList<int>> res = BinaryTreeLevelOrderTraversalClass.LevelOrder(root);
            CollectionAssert.AreEqual(res[0].ToList(), new List<int> { 3 });
        }
    }

    public class BinaryTreeLevelOrderTraversalClass
    {
        public static IList<IList<int>> LevelOrder(TreeNode root)
        {
            List<IList<int>> res = new List<IList<int>>();
            if (root == null)
            {
                return res;
            }
            Queue<TreeNode> Q = new Queue<TreeNode>();

            Q.Enqueue(root);

            int counter = 0; // which level
            while (Q.Count > 0)
            {
                int children = Q.Count;
                res.Add(new List<int>());
                for (int i = 0; i < children; i++)
                {
                    var temp = Q.Dequeue();
                    res[counter].Add(temp.val);
                    if (temp.left != null)
                    {
                        Q.Enqueue(temp.left);
                    }
                    if (temp.right != null)
                    {
                        Q.Enqueue(temp.right);
                    }
                }
                counter++;
            }
            return res;
        }
    }

}

Avoid unnecessary variables

The counter variable is unnecessary.
For each level, you need just a List<int> level,
add the elements of the level to this list,
and then add this list to res.

By eliminating this variable, you also eliminate any concerns about the correctness of res[counter].Add(temp.val);.
With the code changed to level.Add(temp.val),
the mental burden is reduced.

Use more descriptive names

res could be levels
temp could be node
LevelOrder could be GetLevels
BinaryTreeLevelOrderTraversalClass would be better without Class


Stored Procedure enhancement for updating columns in a subset The purpose of this store procedure is to reach into a large database (test), query for a subset, and update a column value, but with a big constraint - I can only update one row at a time in order to avoid deadlocks because the database is large and used by many resources.

The code attempts to change the value of test.[stat] to 'R' if a subset is found with the corresponding stat values.

ALTER PROCEDURE [dbo].testProcedure (@name NVARCHAR(10),
                                    @num INT,
                                    @msg NVARCHAR(200) OUTPUT )

AS

BEGIN

  DECLARE @val1 NVARCHAR(10)
  DECLARE @val2 INT

  SELECT  t.val1, t.val2, t.stat, 'N' rel
  INTO #temp
  FROM   dbo.test t
  WHERE  t.[name] = @name
    AND t.[stat] IN ('A','B')
    AND t.[num] = @num
  IF @@ROWCOUNT = 0
  BEGIN
    SET @msg = 'Nothing returned'
    PRINT @msg
    RETURN -1
  END

  SELECT @val1 = val1 FROM #temp where rel = 'N'

  SET @msg = ' val1: ' +  @val1 + ' '

  WHILE EXISTS (SELECT 1 from #temp WHERE rel = 'N')
    BEGIN
        SELECT @val2 = val2 FROM #temp where rel = 'N'

        UPDATE t
        SET [stat] = 'R'
        FROM   dbo.test t
        WHERE  t.val1 = @val1 AND t.[stat] IN ('A', 'B') AND t.val2 = @val2

        UPDATE #temp
        SET rel = 'Y'
        WHERE val2 = @val2

        SET @msg = @msg + ' - ' + ISNULL(CONVERT(NVARCHAR,@val2),' ')
    END

  IF OBJECT_ID('tempdb..#tempd') IS NOT NULL DROP TABLE #temp

  PRINT @msg
END

Any help on how to improve this code will be greatly appreciated.

Anonymized code
Object/variable names should tell us something about what they actually mean. For instance, these variables:
  DECLARE @val1 NVARCHAR(10)
  DECLARE @val2 INT

What does val1 and val2 actually stand for? In fact everything in the query looks like example code, so it leaves us to guess what the code is actually designed for.

SELECT INTO #table
You would most likely get performance improvements with regards to that temp table if you created it manually instead of using SELECT ... INTO. That would at least let you create some indexes on it. I've gained huge performance improvements with that technique in the past so I strongly recommend explicitly creating your temp tables along with indexes for performance-critical queries.

Cursor
You would likely get better performance in row-by-row operations if you used a cursor, instead of doing an update both to the source table and the temp table in a while loop. See cursors on MSDN for reference.

Print @msg
I'm not really sure why you would print the @msg variable. According to your question, the purpose of the code is:

The code attempts to change the value of test.[stat] to 'R' if a subset is found with the corresponding stat values.

So printing that message to the console seems like a waste of resources. I notice you are returning @msg as output anyways, so if the calling code needs that, it's easy to get it without printing it to the console.


Simplify complex hash-table manipulations in Common Lisp I'm trying to write a simple triplestore in Common Lisp that will store triples in the form subject-predicate-object. The code is inspired by the book "Programming the Semantic Web".

(defvar triplestore-graph (make-hash-table))

(defun triplestore-init ()
  (puthash :spo (make-hash-table) triplestore-graph)
  (puthash :pos (make-hash-table) triplestore-graph)
  (puthash :osp (make-hash-table) triplestore-graph))

(defun triplestore-add (s p o)
  (triplestore-add-to-index :spo s p o)
  (triplestore-add-to-index :pos p o s)
  (triplestore-add-to-index :osp o s p))

(defun triplestore-add-to-index (index a b c)
  (let ((index (gethash index triplestore-graph)))
    (cond ((not (gethash a index))
           (let ((tmp (make-hash-table)))
             (puthash a
                      (progn
                        (puthash b
                                 (list c)
                                 tmp)
                        tmp)
                      index)))
          ((not (gethash b (gethash a index)))
           (puthash b
                    (list c)
                    (gethash a index)))
          (t
           (nconc (list c)
                  (gethash b
                           (gethash a index)))))))

Here (puthash k v table) is defined as (setf (gethash k table) v). The code above is roughly equivalent to the Python code:

graph = {}

def init(graph):
    graph['_spo'] = {}
    graph['_pos'] = {}
    graph['_osp'] = {}

def add(graph, (s, p, o)):
    _addToIndex(graph['_spo'], s, p, o)
    _addToIndex(graph['_pos'], p, o, s)
    _addToIndex(graph['_osp'], o, s, p)

def _addToIndex(index, a, b, c):
    if a not in index:
        index[a] = {b: set([c])}
    elif b not in index[a]:
        index[a][b] = set([c])
    else:
        index[a][b].add(c)

How can i improve readability and elegantness of the Common Lisp code? I'm mainly concerned that operations with hash-tables are verbose comparing to Python's equivalents. Are there macros for hash-table handling, for example?

First of all, I suggest that you use a structure instead of a hash table when you know that the table will only have 3 elements; this improves both readability and performance:

(defstruct triplestore-graph
  (spo (make-hash-table))
  (pos (make-hash-table))
  (osp (make-hash-table)))
(defvar *triplestore-graph* (make-triplestore-graph))

Next, there is no puthash in Common Lisp, use (setf gethash) instead.

Finally, I would modify the code like this:

(defun triplestore-add (s p o)
  (triplestore-add-to-index (triplestore-graph-spo *triplestore-graph*) s p o)
  (triplestore-add-to-index (triplestore-graph-pos *triplestore-graph*) p o s)
  (triplestore-add-to-index (triplestore-graph-osp *triplestore-graph*) o p s)) ; o s p?!

(defun triplestore-add-to-index (index a b c)
  (let ((at (gethash a index)))
    (if at
        (pushnew c (gethash b at))
        (setf (gethash a index)
              (let ((tmp (make-hash-table)))
                (setf (gethash b tmp) (list c))
                tmp)))))


Best way to include image_tag inside link_to What is the most elegant and readable way to in include (potentially long) image_tag calls inside of link_to?

Example

<%= link_to image_tag('buckminsterfullerene.png', width: '210', height: '60', alt: 'Molecular structure of Buckminsterfullerene'), 'some_long_path.html', title: 'Buckminsterfullerene', rel: 'tooltip', relative: true %>

You could create a helper to keep your views less bloated. For example:

app/helpers/application_helper.rb

...
def image_link_to(image_path, url, image_tag_options = { }, link_to_options = { })
  link_to url, link_to_options do
    image_tag image_path, image_tag_options
  end
end
...

app/views/pages/show.html.haml

...
= image_link_to "buckminsterfullerene.png", my_url_helper, { size: '210x60', alt: 'Molecular structure of Buckminsterfullerene' }, { title: 'Buckminsterfullerene', rel: 'tooltip', relative: true }
...


Writing with too many system calls I have a writeOnFile() method with too many fwrite().
I think it is not a good implementation because too many system calls may produce a high overhead.
What can I do?

Code
void writeOnFile()
{
    char cr[1] = {'\n'};    //Usefull characters
    char sp[1] = {' '};

    char header1[] = "Instant        ";             //Headers
    fwrite(header1, 1, strlen(header1), out);
    char header2[] = "Playing (1 yes, 0 no)    ";
    fwrite(header2, 1, strlen(header2), out);
    char header3[] = "Segment ID    ";
    fwrite(header3, 1, strlen(header3), out);
    char header4[] = "Encode level\n";
    fwrite(header4, 1, strlen(header4), out);

    char strNumber[50];                             //Data writing
    for (int i = 0; i < global_index; i++)
    {
        snprintf(strNumber, 50, "%f", instants[i]);
        fwrite(strNumber, 1, strlen(strNumber), out);
        //Adds spaces
        for (int i = 0; i < strlen(header1) - strlen(strNumber); i++)
            fwrite(sp, 1, 1, out);

        snprintf(strNumber, 50, "%d", playing[i]);
        fwrite(strNumber, 1, strlen(strNumber), out);
        for (int i = 0; i < strlen(header2) - strlen(strNumber); i++)
            fwrite(sp, 1, 1, out);

        snprintf(strNumber, 50, "%li", segment_index[i]);
        fwrite(strNumber, 1, strlen(strNumber), out);
        fwrite(sp, 1, 1, out);
        for (int i = 0; i < strlen(header3) - strlen(strNumber) - 1; i++)
            fwrite(sp, 1, 1, out);

        snprintf(strNumber, 50, "%d", encode_level_buff[i]);
        fwrite(strNumber, 1, strlen(strNumber), out);

        //Inserts a \n at the end of the row but not at the end of file
        if (i != global_index - 1)
            fwrite(cr, 1, 1, out);
    }
}

Output example

I have a writeOnFile() method with too many fwrite().  ...

A way to reduce fwrite() calls is to simply write all text to a large char buffer[] and then write that once per for (int i = 0; i < global_index; i++) loop.
Use the return vale of snprintf() to speed calculation of next offset.  Use  *printf() features for padding.
for (int i = 0; i < global_index; i++) {
    char buf[N];
    int offset = 0;

    //snprintf(strNumber, 50, "%f", instants[i]);
    //fwrite(strNumber, 1, strlen(strNumber), out);
    //for (int i = 0; i < strlen(header1) - strlen(strNumber); i++)
    //    fwrite(sp, 1, 1, out);

    //                                           vv---- pad on right
    int len = snprintf(buf+offset, N - offset, "%-*f", (int) (sizeof header1 - 1), instants[i]);
    assert(len >= 0 && len < N - offset);
    offset += len;

    //snprintf(strNumber, 50, "%d", playing[i]);
    //fwrite(strNumber, 1, strlen(strNumber), out);
    //for (int i = 0; i < strlen(header2) - strlen(strNumber); i++)
    //    fwrite(sp, 1, 1, out);

    len = snprintf(buf+offset, N - offset, "%-*d", (int) (sizeof header2 - 1), playing[i]);
    assert(len >= 0 && len < N - offset);
    offset += len;

    ....

    fwrite(buf, 1, offset, out);
}

Some other ideas to linearly improve performance.
Why run down string again to find length?
//snprintf(strNumber, 50, "%f", instants[i]);
//fwrite(strNumber, 1, strlen(strNumber), out);
int len = snprintf(strNumber, 50, "%f", instants[i]);
fwrite(strNumber, 1, len, out);

No need for strlen() for a constant string
char header1[] = "Instant        ";
// fwrite(header1, 1, strlen(header1), out);
fwrite(header1, 1, sizeof header1 - 1, out);

Questionable "safe:" concerns
sprintf() is certainly faster (or as fast) as snprintf() ....
// snprintf(strNumber, 50, "%d", playing[i]);
sprintf(strNumber, "%d", playing[i]);

... yet by some coding standards, better to use snprintf() even though "%d" will not overfill 50 char array.
What is curious about this "safe" code is the lack of checks of fwrite() return, a more likely issue.
    // fwrite(strNumber, 1, strlen(strNumber), out);
    size_t nmemb = strlen(strNumber);
    size_t written = fwrite(strNumber, 1, nmemb, out);
    if (written != nmemb) Handle_Error();

Watch out for unsigned bugs
Consider what happens when strlen(header1) < strlen(strNumber)
for (int i = 0; i < strlen(header1) - strlen(strNumber); i++)

is like
for (int i = 0; i < strlen(header1) + (SIZE_MAX - strlen(strNumber) - 1); i++)

due to unsigned  math wrap around.
Better coded as
for (size_t i = strlen(strNumber); i < strlen(header1); i++)
// or
for (size_t i = strlen(strNumber); i < sizeof header1 - 1; i++)

Watch out for long "%f"
"%f"  may takes 100s  of characters for large FP values.
Consider "%g".

Curious that OP's code did not start with fprintf() rather than snprintf()/fwrite().  This is another option.
Conceptually, fprintf() is effectively doing the char buffer[] mentioned above albeit without many fwrite()/fprintf() calls.
Switching to fprintf()  and using the suggested padding format will be sufficient for a first level speed-up.
For me, I do like the idea of writing a line with one I/O call.   It is symmetric with the idea of reading one line on input code with fgets().


Appending messages to hidden input fields This code contains an array of objects that I loop over and append to hidden input fields in the DOM. As these inputs have no ID, I first select them by name using $.each and then check the value by looping over the array using another $.each. If there is a match, I then append some HTML to the DOM.

It works fine, but I have doubts about the code, especially where I am using two $.each. Is there a better way of doing this?

var messages = [
            {
                "code":"203294641",
                "message":"A night costs 332 miles"
            },
            {
                "code":"203294642",
                "message":"The night is dark and full of terrors. Costs 32 miles."
            },
            {
                "code":"203294643",
                "message":"The night is dark and full of terrors. Costs 67 miles."
            },
            {
                "code":"203294644",
                "message":"The night is dark and full of terrors. Costs 423 miles."
            },
            {
                "code":"203294645",
                "message":"The night is dark and full of terrors. Costs 431 miles."
            },
            {
                "code":"203294646",
                "message":"The night is dark and full of terrors. Costs 76 miles."
            }
        ];

    $('input[name="code"]').each(function(){
        var inp = $(this);
        $.each(messages, function(i) {
            if (inp.val() == messages[i].code){
                inp.closest('.plan').find('.loyalty').append('
' + messages[i].message + '
');
            }
        });
    });

It would be much easier to store the code/message pairs in an object, then no loop is needed:

var messagesByCode = {
  "203294641": "A night costs 332 miles",
  "203294642": "The night is dark and full of terrors. Costs 32 miles."
  // etc.
};

$('input[name="code"]').each(function(){
  var inp = $(this);
  inp.closest('.plan').find('.loyalty').append('
' + messagesByCode[inp.val()] + '
');
});

If you can't change the structure, generate the better structure once at the beginning:

var messagesByCode = messages.reduce(function(object, item) {object[item.code] = item.message}, {});

EDIT: Some more remarks.

Another thing that should be considered is the point of this code. Why are you doing this? Why only once at the beginning? What is the bigger picture?

The reason I ask is, because this looks like something that should happen server-side. Do the input values change client-side? If yes you should structure your code in a way that allows you to update a single input.

What does your HTML look like? Why isn't the

 part of the existing HTML?

I'm not really fond of the hard coded "path" to the target element (....closest('.plan').find('.loyalty')...). I generally prefer to write JavaScript as generic (and reuseable) as possible, but without the bigger picture I can't think of a good alternative right now.


Array of immutable objects initialized at compile-time I have a class that contains defines 'magic' numbers at compile time such as basic array sizes, fixed ip address, etc. so that I have one place to edit if changes are required.

In this nested class I need to define tuples of maximum and minimum values that correspond to each of 4 bands of an IR camera.  I'd like to use the data like this (for readability):

int x = lutBounds[3].min;

Is my code below considered 'good practice'?  If not, why, and what would be better.  I'm completely self-taught in Java but try to use 'best practices' when I can recognize them.

/**
 * LUTBounds holds a band datum for the upper and lower bounds of interest.
 * @author programmer
 *
 */
 static class LUTBounds {
    final int   min;
    final int   max;

    LUTBounds(final int min, final int max) {
        this.min = min;
        this.max = max;

    }

}

// usage: int x = lutBounds[3].min;
final static LUTBounds[]    lutBounds   = { new LUTBounds(14_123, 30_123),
        new LUTBounds(14_922, 40_123), new LUTBounds(14_815, 41_123),
        new LUTBounds(14_300, 40_123) };

I would suggest an enum: they're made to encapsulate a set of distinct values known at compile time. For example, this might be the code you have as a result:

public enum CameraInformation {

    First(14_123, 30_123, "http://google.com"),
    Second(14_922, 40_123, "http://google.com"),
    Third(14_815, 41_123, "http://google.com"),
    Fourth(14_300, 40_123, "http://google.com");

    private final int min, max;
    private final String address;

    CameraInformation(int min, int max, String address) {
        this.min = min;
        this.max = max;
        this.address = address;
    }

    public int getMin() {
        return min;
    }

    public int getMax() {
        return max;
    }

    public String getAddress() {
        return address;
    }
}

The main reason for doing this is because I don't like the 0-based array index with the 1-based cameras: lutBounds[3] refers to camera 4 instead of 3, thus losing some expressiveness even for people familiar with this.

Using an enum also takes care of you having to choose a place to create those boundary values and passing them around: the enum is created once, stores its values in a central place and can be used anywhere you want.

Usage is easy:

int min = CameraInformation.Third.getMin();


A Bee Movie Knock-off I have written a program that takes the original file (the script of 'the bee movie') and does a word count, a unique word count and replaces some of the words. This is really just a rushed version, so if there are any errors or ways of fixing/making the code more efficient, please let me know.
Link to the script: https://github.com/esker-luminous/Bee-Movie-Knock-off/blob/main/script.py - it is a very large file.
Code:
file = open('script.py', 'r')

words = []
punctuation = '1234567890!@#$%^&*()_-+=-/*{}[];:\'"\<>~`|?'

for line in file:
    line = line.lower()
    for char in punctuation:
        line = line.replace(char, '')

    words += line.split()

print(len(words))
print(len(set(words)))
file.seek(0)

text = file.read()
print(text)
file.close()

word_replacement ={'bee': 'turtle', 'hive': 'cow', 'flower': 'algae', 'bees': 'turtle', 'flowers' : 'algae'}
for wor_repl in word_replacement.keys():
    text = text.replace(wor_repl, word_replacement[wor_repl])
file = open('knock off.py', 'w')
file.write(text)
file.close()

For the part where you first read the file, you could write it like this:
import collections
import re

file = open('bee_movie.txt', 'r')

text = ""
words = collections.Counter()
punctuation = '1234567890!@#$%^&*()_-+=-/*{}[];:\'"\\<>~`|?'
all_upper = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
all_lower = "abcdefghijklmnopqrstuvwxyz"
translation = str.maketrans(all_upper, all_lower, punctuation)

for line in file:
    text += line # concatenating like this is optimized in python 3
    words.update(line.translate(translation).split())

print(sum(words.values()))
print(len(words))
file.close()

For the part where you’re replacing words, it would be faster if you used re.sub. When you use text.replace, it needs to remake the entire script every time it’s used.
Also you should save the text files as .txt files, they don’t need the .py extension.
Btw you can import string and use string.ascii_uppercase, string.ascii_lowercase, and string.punctuation if you don’t want to type out all the characters.


Python - username and password authentication How can I reduce the amount of if statements in this program to make it more robust and any other tips for improvement would be grateful.

# importing pickle
import pickle

# defining variables
create_username = 0
create_password = 0
password = 0
username = 0

# this variable allows the user or denies the user to the rest of the program (will only run if access is 1)
access = 0

# creates a users dictionary
with open('users.pickle', 'rb') as f:
    users = pickle.load(f)

print(users)

# sign up (creating new account)
while username not in users and username != 'signup':
    username = input("enter username(type signup to create an account): ")

    # add new user to dictionary
    if username == "signup" or username == "Signup":
        create_username = input("enter a new username: ")
        create_password = input("enter a new password (Your password cannot be the same as your username !!!!!!!): ")

    if create_password in users:
        create_password = input("password taken re-enter: ")
    # then adds the new username to the users dictionary
    if username == 'signup':
        users[create_username] = create_password

if username in users:
    password = input("enter password: ")

if password in users:
    print("access granted")

    access = 1
if username not in users and username != 'signup':
    username = input("enter username: ")

    if username in users:
        password = input("enter password")

    if password in users:
        print("access granted")
        access = 1

if password not in users:
    print("access denied")

with open('users.pickle', 'wb') as f:
    # Pickle the 'data' dictionary using the highest protocol available.
    pickle.dump(users, f, pickle.HIGHEST_PROTOCOL)

print(users)

# importing pickle

Obviously. Don't clutter your code with comments unless they add something we don't already know.

# defining variables
create_username = 0
create_password = 0
password = 0
username = 0

This is generally a bad idea, and you aren't in mid-1990s C. Don't predeclare your variables. Initialize them where they're actually used.

print(users)

If users is a plain-old dictionary, I recommend the use of pprint instead of print.

"enter username(type signup to create an account): "

This is called in-band control, and is a bad idea. What if someone's username is called "signup"? You want out-of-band control. Ask explicitly whether the user wants to sign up or log in.

if username == "signup" or username == "Signup":

Don't do two comparisons. Convert username to lowercase and compare with 'signup'.


Building tree structure based on flat objects You can find here a Follow-up question

Description

A List<UnrelatedObects> is returned by a 3rd party webservice, which will then be transformed to a List<ArchiveDefinition>. These ArchiveDefinition objects are connected by Parent'.ArchiveNodeId == 'Child'.ParentId.

The root objects TypeOfArchive property will always have the value ArchiveType.Archive.

The goal of the given class below is to build a List<ArchiveTreeNode> of these flat object list to fill a treeview control.

The class in question

public class ArchiveBuilder
{
    public static List<ArchiveTreeEntry> Build(List<ArchiveDefinition> entries)
    {
        List<ArchiveTreeEntry> rootArchiveTreeEntries = new List<ArchiveTreeEntry>();

        if (entries != null && entries.Count > 0)
        {
            List<ArchiveDefinition> rootEntries = GetRootEntries(entries);

            foreach (ArchiveDefinition definition in rootEntries)
            {
                rootArchiveTreeEntries.Add(new ArchiveTreeEntry(definition));
                entries.Remove(definition);
            }

            foreach (ArchiveTreeEntry parent in rootArchiveTreeEntries)
            {
                FillChildren(parent, entries);
            }
        }
        return rootArchiveTreeEntries;
    }

    private static void FillChildren(ArchiveTreeEntry parent,
                                     List<ArchiveDefinition> entries)
    {
        if (entries.Count > 0)
        {
            List<ArchiveDefinition> children = GetChildren(entries, parent.Id);

            if (children.Count > 0)
            {
                RemoveChildren(entries, parent.Id);

                foreach (ArchiveDefinition child in children)
                {
                    ArchiveTreeEntry treeEntryChild = new ArchiveTreeEntry(child);
                    parent.AddChild(treeEntryChild);
                    FillChildren(treeEntryChild, entries);
                }
            }
        }
    }

    private static List<ArchiveDefinition> GetRootEntries(List<ArchiveDefinition> entries)
    {
        return entries.FindAll(e => e.TypeOfArchive == ArchiveType.Archive);
    }

    private static List<ArchiveDefinition> GetChildren(List<ArchiveDefinition> entries, string parentID)
    {
        return entries.FindAll(e => e.ParentId == parentID);
    }

    private static void RemoveChildren(List<ArchiveDefinition> entries, string parentID)
    {
        entries.RemoveAll(e => e.ParentId == parentID);
    }
}

Related classes and enums

public class ArchiveDefinition
{
    public string ArchiveNodeId { get; private set; }
    public string ParentId { get; private set; }
    public ArchiveType TypeOfArchive { get; private set; }

    public ArchiveDefinition (String parentId, String archiveNodeId,
                              ArchiveType type)
    {
        ParentId = parentId;
        TypeOfArchive = type;
        ArchiveNodeId = archiveNodeId;
    }
}

public enum ArchiveType
{
    Archive, ArchiveGroup, ArchiveEntry
}

public class ArchiveTreeEntry
{
    public ArchiveType ArchiveEntryType { get; private set; }
    public string Id { get; private set; }
    public ReadOnlyCollection<ArchiveTreeEntry> Children
    {
        get
        {
            return new ReadOnlyCollection<ArchiveTreeEntry>(mChildren);
        }
    }

    private List<ArchiveTreeEntry> mChildren = new List<ArchiveTreeEntry>();
    public ArchiveTreeEntry(ArchiveDefinition archiveDefinition)
    {
        Id = archiveDefinition.ArchiveNodeId;
        ArchiveEntryType = archiveDefinition.TypeOfArchive;
    }

    internal void AddChild(ArchiveTreeEntry child)
    {
        if (child != null)
        {
            mChildren.Add(child);
        }
    }
}

I would like to get a review for the ArchiveBuilder class. If you also want to review the related classes and enums, I won't mind.

You could use the AddRange combined with the Except method inside of Build:

public static List<ArchiveTreeEntry> Build(List<ArchiveDefinition> entries)
{
    List<ArchiveTreeEntry> rootArchiveTreeEntries = new List<ArchiveTreeEntry>();

    if (entries != null && entries.Count > 0)
    {
        List<ArchiveDefinition> rootEntries = GetRootEntries(entries);

        entries = rootArchiveTreeEntried.AddRange(rootEntries.Select(definition=> new ArchiveTreeEntry(definition)).Except(entries);

        foreach (ArchiveTreeEntry parent in rootArchiveTreeEntries)
        {
            FillChildren(parent, entries);
        }
    }
    return rootArchiveTreeEntries;
}

You have made the public method static, which in this case is probably fine but it does make it harder to unit test dependencies. I envisage the class being consumed, like this:

in app

var builder = new ArchiveBuilder();
builder.Build(entities);

That said, the private methods are fine as static methods because the are an implementation detail.
The private methods can return/accept IEnumerable<> instead of List<>
From there, you could then re-write the method above to be:

public static List<ArchiveTreeEntry> Build(List<ArchiveDefinition> entries)
{
    List<ArchiveTreeEntry> rootArchiveTreeEntries = new List<ArchiveTreeEntry>();

    if (entries != null && entries.Count > 0)
    {
        entries = rootArchiveTreeEntried.AddRange(GetRootEntries(entries).Select(entry => new ArchiveTreeEntry(definition)).Except(entries);

        foreach (ArchiveTreeEntry parent in rootArchiveTreeEntries)
        {
            FillChildren(parent, entries);
        }
    }
    return rootArchiveTreeEntries;
}

So here is the completely re-written class:

public class ArchiveBuilder
{
    public IEnumerable<ArchiveTreeEntry> Build(IEnumerable<ArchiveDefinition> entries)
    {
        IEnumerable<ArchiveTreeEntry> rootArchiveTreeEntries = new List<ArchiveTreeEntry>();

        if (entries != null && entries.Count > 0)
        {
            entries = rootArchiveTreeEntried.AddRange(GetRootEntries(entries).Select(entry => new ArchiveTreeEntry(definition)).Except(entries);

            foreach (ArchiveTreeEntry parent in rootArchiveTreeEntries)
            {
                FillChildren(parent, entries);
            }
        }
        return rootArchiveTreeEntries;
    }

    private static void FillChildren(ArchiveTreeEntry parent,
                                 IEnumerable<ArchiveDefinition> entries)
    {
        if (entries.Count > 0)
        {
            IEnumerable<ArchiveDefinition> children = GetChildren(entries, parent.Id);

            if (children.Count > 0)
            {
                RemoveChildren(entries, parent.Id);

                foreach (ArchiveDefinition child in children)
                {
                    ArchiveTreeEntry treeEntryChild = new ArchiveTreeEntry(child);
                    parent.AddChild(treeEntryChild);
                    FillChildren(treeEntryChild, entries);
                }
            }
        }
    }

    private static IEnumerable<ArchiveDefinition> GetRootEntries(IEnumerable<ArchiveDefinition> entries)
    {
        return entries.FindAll(e => e.TypeOfArchive == ArchiveType.Archive);
    }

    private static IEnumerable<ArchiveDefinition> GetChildren(IEnumerable<ArchiveDefinition> entries, string parentID)
    {
        return entries.FindAll(e => e.ParentId == parentID);
    }

    private static void RemoveChildren(IEnumerable<ArchiveDefinition> entries, string parentID)
    {
        entries.RemoveAll(e => e.ParentId == parentID);
    }
}

NB: I haven't compiled the class or tested it, so it might not work out the box.


Changing the ACL on a folder through Wscript.Shell within VBA I have working VBA code in excel that changes a network folders permissions.

The code uses Wscript.Shell to run icacls commands but there are multiple instances of this command and each time it runs, it opens up a new shell window.

It would be interesting to see if there is a way of making the code more efficient by opening a single shell instance then go on to run each of the icacls commands.

Private Sub TestingPermissions()

Dim FSO
Dim MyFolder
Dim objShell

Set FSO = CreateObject("Scripting.FileSystemObject")
Set MyFolder = FSO.GetFolder(Worksheets("Config").Range("D4").Value & ActiveSheet.Range("C21").Value)

Set objShell = CreateObject("Wscript.Shell")

' Take ownership and modify permission of folder
objShell.Run ("takeown /f " & """" & MyFolder & "\My Music""" & " /r /d y")
objShell.Run ("icacls " & """" & MyFolder & "\My Music""" & " /setowner mydomain\admin")
objShell.Run ("icacls " & """" & MyFolder & "\My Music""" & " /grant mydomain\StudentExam101:(OI)(CI)F /T")
objShell.Run ("icacls " & """" & MyFolder & "\My Music""" & " /grant mydomain\DAdmins:(OI)(CI)F /T")
objShell.Run ("icacls " & """" & MyFolder & "\My Music""" & " /grant mydomain\admin:(OI)(CI)F /T")
objShell.Run ("icacls " & """" & MyFolder & "\My Music""" & " /grant SYSTEM:(OI)(CI)F /T")
objShell.Run ("icacls " & """" & MyFolder & "\My Music""" & " /grant CREATOR OWNER:(OI)(CI)F /T")

End Sub

I thought I had found a part solution by combining the icalcs permissions all into one command but further testing and I realised it had not worked, but I will investigate this further anyway.

CodeReview

Why use the Scripting.FileSystemObject?  MyFolder is just returning the folder name

  MyFolder = Worksheets("Config").Range("D4").Value & ActiveSheet.Range("C21").Value

I would also write a function to return the folder path and a second function to create the icacls commands.

Fun Part: My Own Solution

The class below will create a self-deleting batch file.  Running it in silent mode will hide the command window.  The advantage of using a batch file is that you can add a pause that will allow you to inspect the results of your commands.

Note: pause will have no effect in silent mode and the files do not delete themselves immediately but they will automatically delete after a short time.

Class: BatchFile

Attribute VB_Name = "BatchFile"

Option Explicit
Public FileText As String
Private Const DeleteCommand As String = "DEL ""%~f0"""

Public Sub AppendLine(Text As String)
    If Len(FileText) > 0 Then FileText = FileText & vbNewLine
    FileText = FileText & Text
End Sub

Public Sub AddICacls(ByVal FolderName As String, ByVal Parameters As String)
    AppendLine "icacls " & Chr(34) & FolderName & Chr(34) & Parameters
End Sub

Public Sub Execute(SilentMode As Boolean)
    Dim FilePath As String
    FilePath = getTempBatchFileName

    CreateFile FilePath

    Dim oShell As Object
    Set oShell = CreateObject("WScript.Shell")

    If SilentMode Then
        oShell.Run Chr(34) & FilePath & Chr(34), 0
    Else
        oShell.Run Chr(34) & FilePath & Chr(34)
    End If

    Set oShell = Nothing
End Sub

Private Sub CreateFile(FilePath As String)
    Dim Text As String
    Text = FileText & vbNewLine & DeleteCommand

    Dim FileNumber As Long
    FileNumber = FreeFile
    Open FilePath For Output As FileNumber
    Print #FileNumber, Text
    Close FileNumber

    Debug.Print Text
End Sub

Private Function getTempBatchFileName() As String
    Dim n As Long
    Dim FilePath As String

    Do
        n = n + 1
        FilePath = Environ("Temp") & "\" & n & ".bat"
    Loop While Len(Dir(FilePath)) > 0

    getTempBatchFileName = FilePath
End Function

Usage

Sub RunICalcs()
    Const DebugMode As Boolean = True

    Dim Batch As New BatchFile
    Dim FolderName As String
    FolderName = getFolderPath

    Batch.AddICacls FolderName, " /r /d y"
    Batch.AddICacls FolderName, " /setowner mydomain\admin"
    Batch.AddICacls FolderName, " /grant mydomain\StudentExam101:(OI)(CI)F /T"
    Batch.AddICacls FolderName, " /grant mydomain\DAdmins:(OI)(CI)F /T"
    Batch.AddICacls FolderName, " /grant mydomain\admin:(OI)(CI)F /T"
    Batch.AddICacls FolderName, " /grant SYSTEM:(OI)(CI)F /T"
    Batch.AddICacls FolderName, " /grant CREATOR OWNER:(OI)(CI)F /T"

    If DebugMode Then
        Batch.AppendLine "pause"
        Batch.Execute False
    Else
        Batch.AppendLine "pause"
        Batch.Execute True
    End If
End Sub

Function getFolderPath() As String
    getFolderPath = Worksheets("Config").Range("D4").Value & ActiveSheet.Range("C21").Value
End Function


The loop iterates over the whole range(len(M)). It means that the last few products exercise less than 13 digits. It doesn't matter in this particular example (they are all zeros anyway), but may lead to erroneous result in general case. Iterate over range(len(M) - 13).
Digit-to-int conversion happens 13 times for each digit. Better convert them once in a separate loop before doing calculations.
Every time a 0 is encountered, you may safely skip the whole bunch of digits: the product will be 0 anyway.
A sliding window technique may help.

In pseudocode:

product *= data[head]
product /= data[tail]
head += 1
tail += 1

(be careful to not divide by 0).


Convert task data into a flat structure I have an array of data in the below format
let input = [
{
    time: "2020-7",
    tasks: [
      {
        key: "p1",
        value: 15
      },
      {
        key: "p2",
        value: 13
      },
    ]
  },
{
    time: "2020-8",
    tasks: [
      {
        key: "p1",
        value: 16
      },
      {
        key: "p2",
        value: 19
      },
    ]
  },
{
    time: "2020-9",
    tasks: [
      {
        key: "p1",
        value: 12
      },
      {
        key: "p2",
        value: 93
      },
    ]
  }
]

After config input array, then 'dataConvert' is formatted as follows:
dataConvert = [
  ["x","2020-7", "2020-8", "2020-9"],
  ["p1", 15, 16, 12],
  ["p2", 13, 19, 93]
]

I tried use reduce , I want to find another way of optimizing.
let dateConvert = [], valueConvert = [];
data2.forEach(x=>{
   let date = new Date(x.time);
   if (date) {
      let getYear = date.getFullYear();
      let getMonth = date.getMonth() + 1;
      let newDate = `${getYear}-${getMonth}-1`;
      return dateConvert = [...dateConvert, newDate];
   }
})
dateConvert.unshift("x");

// get p1 p2 value
let allTasks = data2.flatMap(x => x.tasks);

 valueConvert = Object.values(allTasks.reduce((arr, item) => {
 arr[item.key] = arr[item.key] || [item.key];
 arr[item.key].push(item.value);
 return arr;
}, {}));
dataConvert = [...[dateConvert], ...valueConvert];

thank u.

From a short review;

The code does not produce exactly your output, the x part is different;
["x", "2020-7-1", "2020-8-1", "2020-9-1"], I assume the code is right
That reduce is fine, it's the best part of the code really
The date part seems complicated the check for the valid date could have been done in a filter, the rest in a map
Consistency is important, you use both the fancy technique dataConvert = [...[dateConvert], ...valueConvert]; and dateConvert.unshift("x"); which could have been dateConvert = ["x", ...dateConvert];, the only good reason for unshift could have been that dateConvert is a const but it isnt
I dont like x as a variable name, it says nothing to me, I would go for o (object)
Similarly when you reduce allTasks you pass arr as the cumulator, but it's not an array, it's an object so you could/should use o there
This [...[dateConvert], ...valueConvert]; is equivalent to [dateConvert, ...valueConvert];, you may want to play around more with the spread operator
get p1 p2 value is a very contemporary comment, it is true today because tasks indeed only contain p1 and p2, but the day that tasks get new or changed, that comment will be worthless, consider something like group all tasks properties
I guess a task is an item, but I would rather see allTasks.reduce((o, task) than allTasks.reduce((arr, item)

If you take all that, I would counter propose this;

function convertData(data){

  let dateConvert = data.filter(o => new Date(o.time)).map(o => {
    const date = new Date(o.time);
    return `${date.getFullYear()}-${ date.getMonth() + 1}-1`;
  });
  dateConvert = ["x", ...dateConvert];

  let allTasks = data.flatMap(x => x.tasks);

  //Group all task properties
   const valueConvert = Object.values(allTasks.reduce((o, task) => {
     o[task.key] = o[task.key] || [task.key];
     o[task.key].push(task.value);
     return o;
   }, {}));

  return [dateConvert, ...valueConvert];
}

const data2 = [
{
    time: "2020-7",
    tasks: [
      {
        key: "p1",
        value: 15
      },
      {
        key: "p2",
        value: 13
      },
    ]
  },
{
    time: "2020-8",
    tasks: [
      {
        key: "p1",
        value: 16
      },
      {
        key: "p2",
        value: 19
      },
    ]
  },
{
    time: "2020-9",
    tasks: [
      {
        key: "p1",
        value: 12
      },
      {
        key: "p2",
        value: 93
      },
    ]
  }
];

console.log(convertData(data2));