Wildcard search in C version 2 A few days ago, I posted my version of a wildcard search algorithm, which you can see here: Wildcard search in C

Today I'm showing version 2. I didn't want to make it in the same post because the code is quite different.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>

#define MULTICHAR '*'
#define ONECHAR '?'
#define null NULL

bool wildcard(const char *value, char *wcard) {

  int vsize = (int)strlen(value);
  int wsize = (int)strlen(wcard);
  bool match = false;

  if (vsize == 0 && wsize == 0) {
  match = true;
  }

  else {
  int v = 0;
  int w = 0;
  int lookAhead = 0;
  bool searchMode = false;
  char search = '\0';

  while (true) {
      if (wcard[w] == MULTICHAR) {
      //starts with * and the value matches the wcard
      if (w == 0 && strcmp(wcard+1,value) == 0) {
          match = true;
          break;
      }
      //the * is the last character in the pattern
      if (!wcard[w+1]) {
          match = true;
          break;
      }
      else {
          //search for the next char in the pattern that is not a ?
          while (wcard[++w] == ONECHAR) {
          lookAhead++;
          }

          //if the next char in the pattern is another * we go to the start (in case we have a pattern like **a, stupid I know, but it might happen)
          if (wcard[w] == MULTICHAR) {
          continue;
          }

          search = wcard[w];
          searchMode = true;
      }
      }

      else {
      if (!value[v] && !wcard[w]) {
          if (searchMode) {
          match = false;
          }
          break;
      }
      if (searchMode) {
          char currentValue = value[v+lookAhead];
          if (currentValue == search) {
          match = true;

          searchMode = false;
          search = '\0';
          lookAhead = 0;
          w++;
          }

          else if (currentValue == '\0') {
          match = false;
          break;
          }

          v++;
          continue;
      }
      else if ((wcard[w] == ONECHAR && value[v] == '\0') || (wcard[w] != value[v] && wcard[w] != ONECHAR)) {
          match = false;
          break;
      }
      else {
          match = true;
      }

      w++;
      v++;
      }
  }
  }

  return match;
}

I think there is still room for improvements, so any advice, tips or critiques are all welcome.

You can find the tests I ran here.

The project can be viewed here.

To compile, just do make wildcard.

I’m afraid your approach to matching multiple characters looks fundamentally misguided to me. Consider cases like:

abcbcd    ab*cd

One approach that works is a recursive matcher, i.e. when encountering *, try wildcard(value+1, wcard) and if that fails, wildcard(value, wcard+1). This will give you the standard semantics of an * matching as much as possible. For a minimal match, reverse the order of the two recursive calls. This is very easy to implement (much easier than your approach, in fact), but tends to need a lot of stack space for large strings.

The alternative is to build an explicit backtracking stack, which is considerably harder to get right, but uses less memory, and usually uses it on the heap rather than the stack.


Create folder on ftp if possible Create folder on ftp if possible.

public async Task<bool> CreateFolder(string path)
{
    FtpWebRequest ftpRequest = null;
    try
    {
        ftpRequest = ftpBuilder.Create(path, WebRequestMethods.Ftp.MakeDirectory);
        using (var ftpResponse = (FtpWebResponse)await ftpRequest.GetResponseAsync())
        using (var ftpStream = ftpResponse.GetResponseStream())
        {
            return true;
        }
    }
    catch
    {
        return false;
    }
}

This is my solution but I have two doubts

Is it ok in this case to have return statement in catch?
Is it ok to have just return true in using?

To answer your questions:

Yes if you never want to know why something went wrong. You should log the exception so you can diagnose issues later. Swallowing exceptions like that is rarely a good idea.
No. Web requests return status codes. It could be returning a 5xx error which would indicate that the directory was not created successfully but you'd be acting like it had worked. A response successfully is not the same thing as a successful response.


Create a tree recursivly I want to create a Treeview in C# which will group file by prefix (here the prefix is a marked by the separator _). The following files should give this tree:
Files list:

p_a
p_a_test
p_LIG
p_p
p_p_c
p_p_c2
p_p_ccc
p_p_test
p_tres
TestLineGraph1
TestLineGrpah

Corresponding tree:

|--p_
    |--p_a
    |--p_a_test
    |--p_LIG
    |--p_p
    |--p_p_
        |--p_p_c
        |--p_p_c2
        |--p_p_ccc
        |--p_p_test
    |--p_tres
TestLineGraph1
TestLineGraph

And how I do it:

 private GraphUINode(List<string> subNodes, GraphUINode parent, string name, int lvl = 0)
        : base(parent.m_viewDataSubControl)
{
    parent.Nodes.Add(this);
    this.Name = name;
    this.Text = name;

    string currentPrefix = "";
    int pertinentSubNodes = 0;
    while (pertinentSubNodes < subNodes.Count -1 && subNodes[pertinentSubNodes].Split('_').Length < 2+ lvl)
        pertinentSubNodes++;

    for (int i = 0; i <= lvl; i++)
    {
        currentPrefix += subNodes[pertinentSubNodes].Split('_')[i] + "_";
    }
    List<String> children = new List<string>();
    foreach (string child in subNodes)
    {
        // The child is in the same group than the previous one
        if (child.StartsWith(currentPrefix))
        {
            children.Add(child);
        }
        else
        {
            // Create a node only if needed
            if (children.Count > 1)
            {
                 // Create the new node
                new GraphUINode(children, this, currentPrefix, lvl + 1);
                children.Clear();
                children.Add(child);
            }
            else
            {
                if (children.Count == 1)
                {
                    new GraphTemplateNode(this, m_viewDataSubControl, children[0]);
                    children.Clear();
                }
                new GraphTemplateNode(this, m_viewDataSubControl, child);
            }
            currentPrefix = "";
            for (int i = 0; i <= lvl; i++)
            {
                currentPrefix += child.Split('_')[i] + "_";
            }
        }
    }
    if (children.Count == 1)
    {
        new GraphTemplateNode(this, m_viewDataSubControl, children[0]);
        children.Clear();
    }
}

but this looks very dirty. How can I improve it?

Is this a bug ?

The correspondending tree you showed is not consistent.  It should look like

|--p_
    |--p_a
    |--p_a_
          |--p_a_test  <- this
    |--p_LIG
    |--p_p
    |--p_p_
        |--p_p_c
        |--p_p_c2
        |--p_p_ccc
        |--p_p_test
    |--p_tres
TestLineGraph1
TestLineGraph

Style

You should always be consistent with the coding style you use. So if you omit (<-bad) braces {} for loops with a single statement, you should stick to it.
You shouldn't shorten variable / parameter names because that removes readability. -> lvl

Comments

Comments should describe why something is done. What is done should be described by the code itself by using meaningful names for classes, methods, parameters.

A comment which describes what is done like

 // Create the new node
new GraphUINode(children, this, currentPrefix, lvl + 1);

should be removed.

Refactoring

Can't be done until the possibility of the bug as mentioned is cleared.

As the root problem to solve is to create a tree structure based on a List<string> I would extract this tree creation outside of the constructor to a separate method which only takes a List<string> as input parameter and is returning something like

class TreeNode
{
    public String Name { get; private set; }
    public IList<TreeNode> ChildNodes { get; private set; }
    public TreeNode()
    {
        ChildNodes = new List<TreeNode>();
    }
    public void FillChildNodes(IList<String> childNodes)
    {

    }
}


Self-inflicted killing utility If you need to kill the same process that you're in (for testing purposes or whatever) this code will do it.

A definitive, quick, unmerciful dead of the current Java program/app.
Not a System.exit(0) or a graceful dead.

A headshot to the running JVM

public class KillMe {

  private static final String CMD_WINDOWS = "taskkill /F /PID %d";
  private static final String CMD_LINUX = "kill -9 %d";

  public static void tryNow()  {
    Thread t = new Thread( () -> {
        try {
            now();
        } catch (CantKillMeException e) {
            //TODO whatever
            e.printStackTrace();
        }
    });
    t.setName("killmethread");
    t.setDaemon(true);
    t.start();
  }

public static void now() throws CantKillMeException {
    int pid = obtainPid();
    String command = getCommand(pid);
    executeCommand(command);
}

static int obtainPid() throws CantKillMeException {
    try {
        java.lang.management.RuntimeMXBean runtime = java.lang.management.ManagementFactory.getRuntimeMXBean();
        java.lang.reflect.Field jvm = runtime.getClass().getDeclaredField("jvm");
        jvm.setAccessible(true);
        sun.management.VMManagement mgmt = (sun.management.VMManagement) jvm.get(runtime);
        java.lang.reflect.Method pid_method = mgmt.getClass().getDeclaredMethod("getProcessId");
        pid_method.setAccessible(true);
        int pid = (Integer) pid_method.invoke(mgmt);
        return pid;
    } catch (IllegalAccessException | java.lang.reflect.InvocationTargetException | NoSuchMethodException | NoSuchFieldException e) {
        throw new CantKillMeException("Cant obtain pid", e);
    }
}

// https://stackoverflow.com/questions/9573696/kill-a-process-based-on-pid-in-java
static String getCommand(int pid) {
    String command =  isWindows() ? CMD_WINDOWS : CMD_LINUX;
    return String.format(command,pid);
}

// https://stackoverflow.com/questions/14288185/detecting-windows-or-linux
static boolean isWindows() {
    String osname = System.getProperty("os.name").toLowerCase();
    return osname.contains("win");
}

static void executeCommand(String command) throws CantKillMeException {
    try {
        Process process = Runtime.getRuntime().exec(command);
        process.waitFor();
    } catch (java.io.IOException | InterruptedException e) {
        throw new CantKillMeException("Cant execute command " + command, e);
    }
}

  public static class CantKillMeException extends Exception {

      CantKillMeException(String message, Throwable cause) {
          super(message, cause);
      }
  }

  }

Will work on Linux. Windows should work (not tested). There's room for improvement, for sure.

Interesting little plot twist in a Java program.

First up, tryNow() is dead code, it's not used, get rid of it, it makes the rest of the class harder to understand by putting red-herrings in the code.

If it is really used somewhere else in your code (it is public static), then it should be moved out of this class in to a more usefully named location.... and it has other issues too, which I will ignore... except for .... never extend Thread directly, use Runnable ....

Next up, you should ensure you are using Java 9.x or newer, and then use the more convenient ProcessHandle API for getting your current processID

This reduces the code:

static int obtainPid() throws CantKillMeException {
    try {
        java.lang.management.RuntimeMXBean runtime = java.lang.management.ManagementFactory.getRuntimeMXBean();
        java.lang.reflect.Field jvm = runtime.getClass().getDeclaredField("jvm");
        jvm.setAccessible(true);
        sun.management.VMManagement mgmt = (sun.management.VMManagement) jvm.get(runtime);
        java.lang.reflect.Method pid_method = mgmt.getClass().getDeclaredMethod("getProcessId");
        pid_method.setAccessible(true);
        int pid = (Integer) pid_method.invoke(mgmt);
        return pid;
    } catch (IllegalAccessException | java.lang.reflect.InvocationTargetException | NoSuchMethodException |
NoSuchFieldException e) {
        throw new CantKillMeException("Cant obtain pid", e);
    }
}

to just:

static int obtainPid() throws CantKillMeException {
    try {
       return ProcessHandle.current().pid();
    } catch (SecurityException e) {
        throw new CantKillMeException("Cant obtain pid", e);
    }
}

Although you don't want to do a System.exit(0) I would still plan it as a backup...

You have

static void executeCommand(String command) throws CantKillMeException {
    try {
        Process process = Runtime.getRuntime().exec(command);
        process.waitFor();
    } catch (java.io.IOException | InterruptedException e) {
        throw new CantKillMeException("Cant execute command " + command, e);
    }
  }

which anticipates exceptions on something that should terminate the process....

I would instead log the exception (actually, catch any Throwable), and exit.... within a finally-block - I would do that in the now() method:

public static void now() throws CantKillMeException {
    try {
        int pid = obtainPid();
        String command = getCommand(pid);
        executeCommand(command);
    } catch (Throwable t) {
        System.out.println(t);
    } finally {
        System.exit(1);
    }
}

You can choose a different (improved) logging mechanism.


Connected components algorithm does not scale past 10,000 nodes I found a connected components algorithm posted here on SO and modified it for my own purposes. It runs well for small input sets, but doesn't scale as well as I would like.

The largest set I've been able to handle with it has just under 10,000 nodes and took 109 seconds to process. I threw in the towel when testing a set of about 107,000 nodes after 30 minutes at 100% CPU load (RAM utilization <10%).

I have several sets in the 100,000- to 300,000-node range in which I'd like to identify connected components. Is there some way I could modify this algorithm to scale better?

Node = collections.namedtuple('Node', 'fact_id person_ids names')

def grouped(neighborhood):
    """takes a flat set of nodes and returns the connected subsets"""
    seen = set()
    def component(node):
        unseen_nodes = set((node,))
        while unseen_nodes:
            node = unseen_nodes.pop()
            seen.add(node)
            unseen_nodes |= adjacent(node, neighborhood) - seen
            yield node
    return (set(component(node)) for node in neighborhood if node not in seen)

def adjacent(a, n):
    """returns all elements of n(eighborhood) adjacent to a(ctive node)"""
    return {e for e in n if a.person_ids & e.person_ids or a.names & e.names}

My background is not in computer science but I'm willing to take some time to study and understand an alternative approach, within reason.

I can't provide my source data but here's set of example Nodes comprising four connected components, if that's helpful:

examples = set([
    Node(49559, frozenset(['62169']), frozenset(['COX'])),
    Node(56669, frozenset(['62169', '70260']), frozenset(['COX', 'HUGHES'])),
    Node(71440, frozenset(['87830', '87829']), frozenset(['WARREN'])),
    Node(267031, frozenset(['303253']), frozenset(['FOX'])),
    Node(324771, frozenset(['358566']), frozenset(['FOX'])),
    Node(405519, frozenset(['427822']), frozenset(['EVANS'])),
    Node(437153, frozenset(['452540', '452541']), frozenset(['BOYD'])),
    Node(474774, frozenset(['482316']), frozenset(['WARREN'])),
    Node(509974, frozenset(['509895', '509894']), frozenset(['BOYD'])),
    Node(552313, frozenset(['544087', '544086']), frozenset(['BOYD'])),
    Node(566768, frozenset(['555467', '555468']), frozenset(['GARDNER', 'WARREN'])),
    Node(603318, frozenset(['583882', '583883']), frozenset(['WARREN'])),
    Node(886519, frozenset(['891668']), frozenset(['FOX'])),
    Node(951212, frozenset(['973755', '973754']), frozenset(['COX'])),
    Node(1043850, frozenset(['1090122']), frozenset(['GARDNER'])),
    Node(1136082, frozenset(['1204393', '1204394']), frozenset(['WILSON', 'BOYD'])),
    Node(28161119, frozenset(['1204393']), frozenset(['WILSON'])),
    Node(1343476, frozenset(['973755', '973754']), frozenset(['COX'])),
    Node(1347042, frozenset(['1457174']), frozenset(['BOYD'])),
    Node(1469345, frozenset(['1597595']), frozenset(['GARDNER'])),
    Node(1561624, frozenset(['1701196', '1701195']), frozenset(['GARDNER', 'EVANS'])),
    Node(1643273, frozenset(['1783604', '1783603']), frozenset(['GARDNER'])),
    Node(1801071, frozenset(['1963032', '1963031']), frozenset(['CHAPMAN', 'EVANS'])),
    Node(1836980, frozenset(['2004300', '2004301']), frozenset(['BOYD'])),
    Node(10298940, frozenset(['2004300', '2004301']), frozenset(['BOYD'])),
    Node(1843190, frozenset(['2011505']), frozenset(['CHAPMAN'])),
    Node(1905248, frozenset(['2082776']), frozenset(['GARDNER'])),
    Node(1927031, frozenset(['2108156', '2108157']), frozenset(['FOX'])),
    Node(2039106, frozenset(['2236190', '2236191']), frozenset(['FOX'])),
])

In algorithm implementation there are usually subtleties that are difficult to spot in a code review. In these cases, I always recommend to have a look at the implementation in other libraries in which smarter people have already spent time to get a good performance.

In this case, I think the library to look at is NetworkX and the implementation for the algorithm you're working on is here.

I hope this helps.


Producer Consumer in C I'm doing multithreading at university and we've been asked to implement the producer consumer problem in C.

My code compiles and produces the right result. From debugging it, it seems to be waiting for the semaphores to be freed etc. I'm using Xcode 6.4, hence the need for sem_open() instead of sem_init(). I just have a few questions regarding my code.

Have I done it correctly? Are there any mistakes/bugs?
If I wanted to make buff larger than one, would I set the semaphore fre to have an initial value of size buff? In the producer thread, would I have to call sem_wait(fre) bufftimes before I called sem_post(busy)? And then a similar thing in the consumer thread?

This is my code:

#include <stdio.h>
#include <stdlib.h>
#include <semaphore.h>
#include <pthread.h>
#include <errno.h>
#include <string.h>

void * producer(void * b);
void * consumer(void * b);

void error(char msg) {
    perror(&msg);
    exit(1);
}

sem_t * fre, * busy;

int size_buff = 1;
char message[] = "Hello!";
int size_msg = sizeof(message);

int main(int argc, const char * argv[]) {

    //declare threads
    pthread_t prod,con;
    int r;

    //create buffer and place it on the stack
    char * buff;
    buff = (char *) malloc(sizeof(char) * size_buff);

    //declare semaphores and initilise them
    char prod_name[] = "/Prod";
    char con_name[] = "/Con";
    fre = sem_open(prod_name, O_CREAT, 0777, 1);
    if ((int) fre == -1)
        error(errno);
    busy = sem_open(con_name, O_CREAT, 0777, 0);
    if ((int) fre == -1)
        error(errno);

    //start threads
    r = pthread_create(&prod, NULL, producer, (void *) buff);
    if (r) {
        error(r);
    }
    r = pthread_create(&con, NULL, consumer, (void *) buff);
    if (r) {
        error(r);
    }

    //unlink semaphores
    sem_unlink(prod_name);
    sem_unlink(con_name);

    pthread_exit(NULL);
    return 0;
}

void * producer(void * b) {
    char * buff = (char *) b;
    int count = 0;

    //loop through message[] and write elements to buffer
    while (count < size_msg) {
        sem_wait(fre);
        strncat(buff,&message[count],1);
        sem_post(busy);
        count++;
    }

    return NULL;
}

void * consumer(void * b) {
    char * recv;
    recv = malloc(sizeof(char) * size_msg);
    char * buff = (char *) b;
    int count = 0;

    //read from the buffer
    while (count < size_msg) {
        sem_wait(busy);
        strncat(recv, buff, 1);
        memmove(buff, buff + 1, 1); //move the elements in the buffer along one, essentailly deleting the element just read
        sem_post(fre);
        count++;
    }
    printf("%s",recv);
    return NULL;
}

When calling the function malloc():

The returned type is void*, so it can be assigned to any pointer, making the 'cast' totally unneeded. It just clutters the code, making it more difficult to understand, debug, maintain.
The expression sizeof(char) is defined in the standard at 1. Multiplying anything by 1 has no effect, so the expression just clutters the code.
When calling any of the memory allocation functions (malloc, calloc, realloc), always check (!=NULL) the returned value to assure the operation was successful.

When invoking the main() function and the code does not use the parameters, use the signature int main(void).

When compiling, always enable all the warnings, then fix those warnings. (For gcc, at a minimum use: -Wall -Wextra -pedantic I also use: -Wconversion -std=gnu99.)

For ease of understanding and readability by the reader of the code:

Separate code blocks (for, if, else, while, do...while, switch, case, default) via a blank line.
Use meaningful variable names. A variable name should indicate usage or content (or better, both).

The code is using 'named' semaphores. 'named' semaphores are persistent, so calling sem_open() with the O_CREAT parameter will fail if the file has ever been run before.  I suggest first calling sem_unlink().

The calls to error() are not correct. The passed parameter is errno, a negative number, not a char and will not tell the user anything useful, especially if what is displayed is the contents of memory those address corresponds to the errno value. I suggest calling syserror() to get the error message that the OS thinks caused the error.

The call to pthread_create() returns 0 on success and -1 on failure. Passing that -1 to the function error() is meaningless.

I suggest passing a char* similar to error("pthread_create for producer failed") then modifying the error() function to expect a pointer to a char array.

The call to sem_open() returns a value SEM_FAILED which might be equal to -1 but that is an implementation detail, so do not use -1, use SEM_FAILED.

The second call to sem_open() returns busy, so the error check following that call should be checking busy, not free.

There are several other problems with the code, but the above comments should get you moving in the right direction.


Remove and move elements in a WebBrowser Control page I'm making some changes to a page on a WebBrowser Control. There is some indications in the code. I just want to make sure that I don't have any unnecessary lines of code.

namespace AL4
{
    public partial class Form1 : Form
    {
        public Form1()
        {
            InitializeComponent();
        }

        private void webBrowser1_DocumentCompleted(object sender, WebBrowserDocumentCompletedEventArgs e)
        {
            webBrowser1.Document.GetElementById("gpu_notice").Style = "display:none"; // hide gpu_notice element
            webBrowser1.Document.GetElementById("header").OuterHtml = ""; // delete header element
            webBrowser1.Document.GetElementById("ads").OuterHtml = ""; // delete ads element
            webBrowser1.Document.GetElementById("the_game").Style += "position: absolute; top: 0px; right: 0px;"; // align the_game element to the center of the webbrowser
        }
    }
}

I think all your 4 lines of code are very necessary in your application, but you forgot for example about error handling. If one of your four IDs disappear you'll get a juicy exception.


Using Guava to reduce code complexity (and possibiliy to improve readability) of null check and assign default value Below is my java code with some complexity,

String input;
double a;

if (null != input && !input.isEmpty()) {
    a = Double.parseDouble(input);
} else {
    a = defaultValue;
}

With Guava, we can compress the code from 5 lines into a 'single' line,

a = Double.parseDouble(
        Objects.firstNonNull(Strings.emptyToNull(input), defaultValue)
    );

The complexity is reduced but readability is getting worst. Any comment on improving readability at the same time reducing complexity?

This is a 'specialized' task, converting an input string to a double value. Whenever I have micro-tasks like this I try to refactor them in to a method which does things properly.

As far as I am concerned, neither of the above two systems are valid. You should be validating the input value to make sure that, even if it is populated with a value, that the value is meaningful, and the parse succeeds.

So, I would have a function:

private static final double inputToDouble(String input, double defaultval) {
    if (input == null || input.isEmpty()) {
        // clear out the obviusly invalid values
        return defaultval;
    }
    try {
        return Double.parseDouble(input.trim());
    } catch (NumberFormatException nfe) {
        // silently ignore invalid data, and return the default value
        return defaultval;
    }
}

Sure, the above may be a bit more complicated, but, using it is a charm....

a = inputToDouble(input, defaultValue);

Sometimes applications do this sort of thing a lot, and in those cases I centralize these micro-tasks to static classes like:

public final class InputUtils {
    private InputUtils() {
        // nothing
    }

    public static final double inputToDouble(......) {....};

    .....

}

And then you can reference these micro-tasks from many places, perhaps even doing a static import on the class.


Detect that a class is marked by a custom annotation in Spring I would like to enable application behaviour based on the presence of a custom annotation that marks the configuration class (normally named something like ApplicationConfig.class) in a Spring 4 application.

This feature functions in the same way as do @EnableCache, @EnableRepositories etc, in the sense that adding these annotations adds behaviour to the application.

The following code shows the way I have decided to implement the feature. It detects and analyses the classes that are loaded by Spring at application startup and fires an event if it determines that one of those classes is annotated with the specified custom annotation. This code works as designed however the use of reflection feels clumsy and I would like to seek the advice of other developers who may have solved a similar problem in a more elegant/different manner.

The custom annotation class.

@Target({ElementType.TYPE})
@Retention(RetentionPolicy.RUNTIME)
public @interface ServiceRegistration {
    boolean enabled() default true;
}

The configuration class.

@ServiceRegistration(enabled = true)
@Configuration
@ComponentScan(basePackageClasses = Application.class, excludeFilters = @Filter({Controller.class, Configuration.class}))
@EnableJpaRepositories("com.package.repository")
class ApplicationConfig {

    @Bean
    public static PropertyPlaceholderConfigurer propertyPlaceholderConfigurer() {
        PropertyPlaceholderConfigurer ppc = new PropertyPlaceholderConfigurer();
        ppc.setLocation(new ClassPathResource("/persistence.properties"));
        return ppc;
    }

}

The class that detects the presence of the annotation.

@Component
public class ServiceRegistrationEventListener {

    private static final String ANNOTATED_CLASSES = "annotatedClasses";

    @Autowired
    ApplicationEventPublisher applicationEventPublisher;

    @Autowired
    ApplicationContext applicationContext;

    @EventListener
    void contextRefreshedEvent(ContextRefreshedEvent event) throws NoSuchFieldException, IllegalAccessException {

        ApplicationContext applicationContext = event.getApplicationContext();
        AnnotationConfigWebApplicationContext annotationContext = ((AnnotationConfigWebApplicationContext) applicationContext);
        for (Field field : annotationContext.getClass().getDeclaredFields()) {
            if (ANNOTATED_CLASSES.equals(field.getName())) {
                field.setAccessible(true);
                Object aContext = field.get(annotationContext);
                if (aContext instanceof Set) {
                    Set<Class<?>> classes = (Set<Class<?>>) aContext;
                    for (Class clazz : classes) {
                        for (Annotation annotation : clazz.getDeclaredAnnotations()) {
                            if (annotation.annotationType().isAssignableFrom(ServiceRegistration.class)) {
                                applicationEventPublisher.publishEvent(new RegistrationEvent(((ServiceRegistration) annotation).enabled()));
                            }
                        }
                    }
                }
            }
        }
    }

    class RegistrationEvent {

        private boolean enabled;

        public RegistrationEvent(boolean enabled) {
            this.enabled = enabled;
        }

        public boolean enabled() {
            return enabled;
        }

    }

}

Values set in the annotation e.g. @ServiceRegistration(enabled = true) are recovered and passed into the published event.

Any ideas on how to improve this code and especially ideas on completely different ways to achieve the same are welcome.

I'm not that familiar with Spring, but if your goal is to get all types annotated with FooBar and then the enabled attribute, you may just use the Reflections library.
When your application is bootstrapping you would:

Reflections reflections = new Reflections("my.project");

And in your listener:

@EventListener
void contextRefreshedEvent(ContextRefreshedEvent event) throws NoSuchFieldException, IllegalAccessException {
    Set<Class<?>> classes = reflections.getTypesAnnotatedWith(FooBar.class);
    for (Class clazz : classes) {
        boolean enabled = clazz.getAnnotation(FooBar.class).enabled();
        System.out.println("Enabled: " + enabled);
    }
}

Notice that you don't need to get all annotations in the class, check types and then cast if you use getAnnotation. The advantage of using Reflections is that class metadata is indexed, which means the expensive reflection operations aren't done all the time. It won't work, however, if Spring changes annotations on runtime (which I don't know if it does or not). If you can't use the Reflections library, just the getAnnotation method is already an improvement over your original for over each annotation.


Presenting modal dialogs from XIB in Cocoa: best/shortest pattern? Below is my typical WindowController module for presenting a modal dialog (could be settings, asking username/password, etc) loaded from a XIB. It seems a bit too complex for something like this. Any ideas how this can be done better/with less code?

Never mind that it's asking for a password, it could be anything. What frustrates me most is that I repeat the same pattern in each and every of my XIB-based modal window modules. Which of course means I could define a custom window controller class, but before doing that I need to make sure this is really the best way of doing things.

#import "MyPasswordWindowController.h"

static MyPasswordWindowController* windowController;

@interface MyPasswordWindowController ()
@property (weak) IBOutlet NSSecureTextField *passwordField;
@end

@implementation MyPasswordWindowController
{
    NSInteger _dialogCode;
}

- (id)init
{
    return [super initWithWindowNibName:@"MyPassword"];
}

- (void)awakeFromNib
{
    [super awakeFromNib];
    [self.window center];
}

- (void)windowWillClose:(NSNotification*)notification
{
    [NSApp stopModalWithCode:_dialogCode];
    _dialogCode = 0;
}

- (IBAction)okButtonAction:(NSButton *)sender
{
    _dialogCode = 1;
    [self.window close];
}

- (IBAction)cancelButtonAction:(NSButton *)sender
{
    [self.window close];
}

+ (NSString*)run
{
    if (!windowController)
        windowController = [MyPasswordWindowController new];
    [windowController loadWindow];
    windowController.passwordField.stringValue = @"";
    if ([NSApp runModalForWindow:windowController.window])
        return windowController.passwordField.stringValue;
    return nil;
}

The application calls [MyPasswordWindowController run], so from the point of view of the user of this module it looks simple, but not so much when you look inside.

Based on Ken Thomases answer given on SO here, here is my revised modal window module. Note that the window should have the system close button disabled, or otherwise you will have to handle windowWillClose, which will lead to having _dialogCode like in my original code above. If, however, OK and Cancel are the only buttons that end the modal dialog, then:

#import "MyPasswordWindowController.h"

@interface MyPasswordWindowController ()
@property (weak) IBOutlet NSSecureTextField *passwordField;
@end

@implementation MyPasswordWindowController

- (IBAction)okCancelAction:(NSButton *)sender
{
    [NSApp stopModalWithCode:sender.tag]; // the OK button's tag should be 1
    [self.window close];
}

+ (NSString*)run
{
    MyPasswordWindowController* windowController =
        [[MyPasswordWindowController alloc] initWithWindowNibName:@"MyPassword"];
    if ([NSApp runModalForWindow:windowController.window])
        return windowController.passwordField.stringValue;
    return nil;
}

@end


Function in Python to extract web Data I developed this feature that I think can be improved quite a bit. The result is the desired one, but it took me many lines of code. Any idea to optimize it?
import json
import urllib.request

dni = 71179047

def get_cuit(dni):
    request = urllib.request.Request('https://www.cuitonline.com/search.php?q=' + str(dni))
    response = urllib.request.urlopen(request)
    data_content = response.read()
    html = data_content.decode('ISO-8859-1')
    cuit = re.findall(r"\(?\b[0-9]{2}\)?-?[0-9]{7,8}-[0-9]{1}\b", html)[0]
    result = re.sub('[^0-9]','', cuit)
    return result

get_cuit(dni)

Use Requests for HTTP and let it deal with the encoding
Do not bake the query into the URL string
Use BeautifulSoup for HTML parsing
Use typehints

Nice and simple:
from requests import get
from bs4 import BeautifulSoup

def get_cuit(dni: int) -> str:
    with get(
        'https://www.cuitonline.com/search.php',
        params={'q': dni},
    ) as resp:
        resp.raise_for_status()
        doc = BeautifulSoup(resp.text, 'html.parser')

    span = doc.find('span', class_='cuit')
    return span.text

print(get_cuit(71_179_047))


Permutations Finder Help Number 2 Previous question:

Java application for finding permutations efficiently>

I have changed quite a bit of code, and need some more reviews.

class Permutations {

    static long factorial(int num){
        long factorial = num;
        for (int forBlockvar = num; forBlockvar > 1; forBlockvar--) {
            factorial = factorial * forBlockvar;
        }
        return factorial / num;
    }

    public static void main(String[] args){
        long FactNmR;
        int n = 8;
        int num = n;
        int r = 6;
        int nMr = n - r;

        long FactN = factorial(num);

        if (nMr == 2) {
            FactNmR = 2;
        }
        else if (nMr <= 1){
            FactNmR = 1;
        }
        else if (nMr >= 2) {
            num = nMr;
            FactNmR = factorial(num);
        }

        long permutations = FactN;

        permutations = permutations / FactNmR;

        System.out.println(permutations);
    }
}

Code comment:

static long factorial(int num){
    long factorial = num;
    for (int forBlockvar = num; forBlockvar > 1; forBlockvar--) {
        factorial = factorial * forBlockvar;
    }
    return factorial / num;
}

You are starting your factorial = num, then multiplying it by num in the first iteration of the loop, then dividing by num at the end.  Starting with factorial = 1, or starting  forBlockvar = num - 1 (I prefer the second) will remove the need to divide at the end.  You can also return 1 if the input is less than 2.

static long factorial(int num){
    if(num < 2) {
        return 1;
    }
    long factorial = num;
    for (int forBlockvar = num - 1; forBlockvar > 1; forBlockvar--) {
        factorial = factorial * forBlockvar;
    }
    return factorial;
}

You could also implement this function recursively.

static long factorial(int num){
    if(num < 2) {
        return 1;
    }
    return num * factorial(num - 1);
}

General:

What do you mean by finding the permutations of 2 numbers?  There are 2 permutations of 2 numbers [x, y]: [x, y] and [y, x].

EDIT

I think I know what you are trying to do now... You want to know how many permutations are available by selecting r objects out of a total of n.

The basic formula for that is n! / (n-r)!.  However, this means calculating the factorial of (n-r) twice (in our example, we have 8*7*6*5*4*3*2*1 / 2*1, but with a lower r the overlap will be greater).

Instead of calculating the complete factorial for both numerator and denominator, there is a way to calculate the value of the numerator such that the denominator is always going to be 1.  That means we eliminate one factorial calculation, and reduce the other.  I'm going to let you think about it a bit before just giving the answer.

EDIT 2

Since you have figured out what I was hinting at, I'll explain it further.

n! can be represented as n*(n-1)*(n-2)*(n-3)...(2)*(1).  When r is less than n and greater than 0, n! can be then be represented as n*(n-1)*(n-2)...(n-r+1)*(n-r)*(n-r-1)*(n-r-2)...(2)*(1).

(n-r)! is (n-r)*(n-r-1)*(n-r-2)...(2)*(1).

Therefore, our numerator is n*(n-1)*(n-2)...(n-r+1)*(n-r)*(n-r-1)*(n-r-2)...(2)*(1), and our denominator is (n-r)*(n-r-1)*(n-r-2)...(2)*(1).  Cancelling like terms, we have n!/(n-r)! = n*(n-1)*(n-2)...(n-r+1).

static long permute(int totalItems, int numToPick) {
    long permutations = totalItems;
    while (--numToPick > 0) {
        permutations *= --totalItems;
    }
    return permutations;
}

You should ensure that 0 < numToPick <= totalItems, and totalItems > 0.


.NET REST API JSON Filtering My question, REST API and lazy loading objects, didn't receive any answers (or many views), so here is my attempt at the solution.

Problem overview:
My mobile app requests a list of objects through a HTTP REST API but only required a small subset of each item to display items in a list. Other larger properties should not be returned unless requested i.e. in an an object detailed view.

My attempt:
API will act as normal unless a querystring is appended with the fields that are required.

I know that I need to fix the requiredFields.Contains(property.Name) call to ignore case during comparison.

My other concern is that this code is redundant due to my use of Automapper that may include functionality similar to this.

Also, could it be more efficient?

public static Object FilterFields(Object target, Uri requestUri)
    {
        NameValueCollection mapQuery = UriExtensions.ParseQueryString(requestUri);

        IEnumerable<string> requiredFields = null;

        if (mapQuery != null && mapQuery.Get("fields") != null)
            requiredFields = mapQuery.Get("fields").Split(new char[] { ',' });

        if (requiredFields != null)
        {
            foreach (PropertyInfo property in target.GetType().GetProperties())
            {
                if (!requiredFields.Contains(property.Name))
                {
                    property.SetValue(target, null);
                }
            }
        }

        return target;
    }

Style

Be consistent in your style. If you use braces {} for single if statements then you should use them always.

Using guard conditions help you to save horizontal space.

NameValueCollection mapQuery = UriExtensions.ParseQueryString(requestUri);
if (mapQuery == null) { return target; }

string fields = mapQuery.Get("fields");
if (String.IsNullOrEmpty(fields)) { return target; }

IEnumerable<string> requiredFields = fields.Split(new char[] { ',' });
if (requiredFields == null) { return target; }

your loop here

otherwise your code looks good.


List union implementation I'm trying to implement union from the standard Data.List library. I know it's reinventing the wheel, but reimplementing functions is how I learn things. Any feedback at all would be helpful!

union' :: (Eq a) => [a] -> [a] -> [a]
union' [] _ = []
union' _ [] = []
union' xs ys = nub' $ xs ++ ys where
    nub' [] = []
    nub' (x:xs)
        | not $ x `elem` xs = x : nub' xs
        | otherwise = nub' xs

This code works, but it strikes me as verbose. union is the opposite of intersect, which does not require a helper where statement:

intersect' :: (Eq a) => [a] -> [a] -> [a]
intersect' [] _ = []
intersect' _ [] = []
intersect' (x:xs) list
    | x `elem` list = x : intersect' xs list
    | otherwise = intersect' xs list

How can I make my union function resemble the elegance of my intersect function?

Revision here

Your base cases are both invalid and unnecessary:

union' [] _ = []
union' _ [] = []

There isn't anything bad about using where, especially when you consider that union' needs no base cases.  Your question is, really, whether union' should be implemented using some kind of nub or not.  If you're reinventing-the-wheel as an exercise, why wouldn't you want to reimplement nub as well?

The real question you should be asking is how to avoid ++.


Create a WordPress site data with Nginx app data automatically The following script is aimed for Ubuntu >=16.04 LEMP environments with PHP-FPM >=7.0, MySQL, WP-CLI, Certbot, and WordPress apps.

The script get's one domain as an argument and creates the following aspects based on that argument (drt=document-root):

WP_app directory in drt.
WP_installation in drt/domain.
WP_conf in drt/domain.
NX_app conf in /etc/nginx/sites-available/
NX_symlink in /etc/nginx/sites-enabled/
NX_certbot configuration.

${drt} = document root.

One could then add a database stack via MySQL, accordingly (and also edit the website files, accordingly).

#!/bin/bash
domain="$1" && test -z ${domain} && return
read -sp "DB user password:" dbuserp

wp core download --path=${drt}/${domain}/ --allow-root # REDUNDANTS MKDIR
wp config create --path=${drt}/${domain}/ --dbname=${domain} --dbuser=${domain} --dbpass=${dbuserp} --dbhost="localhost" --allow-root

cat ~/myAddons/nginx_app > ${sava}/${domain}.conf
sed -i "s/\${domain}/${1}/g" /${sava}/${domain}.conf

ln -sf ${sava}/${domain}.conf ${sena}
certbot --nginx -d ${domain} -d www.${domain}

chown -R www-data:www-data ${drt}/
chmod -R a-x,a=rX,u+w ${drt}/
/etc/init.d/php*-fpm restart && systemctl restart nginx.service

echo "Please configure DBstack in phpmyadmin."
echo "Please change permissions as you prefer and restart the Nginx server."

Clarification

The conf template I redirect from ~/nginx_app, for the Nginx app conf, is this one:

server {
    root ${drt}/${domain}/;
    server_name ${domain} www.${domain};
    location ~* \.(jpg|jpeg|png|gif|ico|css|js|ttf|woff|pdf)$ { expires 365d; }
}

Please share your opinion on the above script and adjacent conf file.

Shellcheck.net

I strongly recommend to start using https://shellcheck.net/.
Simply copy-paste your script,
and it will tell you many issues with this code.

Don't use return outside of functions

The return statement only works within functions.
You probably meant exit instead:

domain="$1" && test -z "${domain}" && exit

Variables in command arguments

Always double-quote variables in command arguments to prevent glob expansion and word splitting, for example:

cat ~/myAddons/nginx_app > "${sava}/${domain}.conf"

Inconsistent paths

Here, the last argument of the second command starts with a /, but not the last argument of the first command:

cat ~/myAddons/nginx_app > ${sava}/${domain}.conf
sed -i "s/\${domain}/${1}/g" /${sava}/${domain}.conf

I suspect the last arguments are intended to be the same in both of these commands. This is confusing.


Control class for an entity For a Student object in the package entity, I have a control.studentpackage with classes that are relevant to the Student object.

The Student is a class with only private properties and getters/setters for every property. The setters do no validation on their own.

My project structure is as follows:

src/
  control/
    student/
      StudentControl.java
      StudentValidator.java
      ValidationError.java
  entity/
     Student.java

ValidationError is just a String wrapper with getters and setters.

The StudentValidator is a functional interface that extends Function<Student, Optional<ValidationError>>.

interface StudentValidator extends Function<Student, Optional<ValidationError>> {}

The StudentControl class is as follows:

package control.student;

import java.util.ArrayList;
import java.util.Date;
import java.util.List;
import java.util.Optional;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import entity.Student;

public class StudentControl {

  private static final Pattern namePattern = Pattern.compile("[aA-zZ]+");

  /**
   * List of functions that validate a Student.
   */
  private static final List<StudentValidator> validatorFunctions = new ArrayList<>();
  static {
    validatorFunctions.add(validateNamesFunction());
  }

  /**
   * Validates a Student's fields.
   *
   * @param student Student to validate
   * @return an Optional that may contain a List of ValidationError. Those ValidationError(s) will
   *         have error messages describing each error occurred while validating the student.
   */
  public static Optional<List<ValidationError>> validate(Student student) {
    List<ValidationError> errors = validatorFunctions.stream()
        .map(f -> f.apply(student))
        .filter(Optional::isPresent)
        .map(Optional::get)
        .collect(Collectors.toList());

    return errors.isEmpty() ? Optional.empty() : Optional.of(errors);
  }

  /**
   * Returns a function that validates a Student's name.
   */
  public static StudentValidator validateNamesFunction() {
    return student -> {
      return namePattern.matcher(student.getName()).matches()
          ? Optional.empty()
          : Optional.of(new ValidationError("Invalid name"));
    };
  }

  /**
   * Returns a function that validates a Student's birth date.
   */
  public static StudentValidator validateBirthDateFunction() {
    return student -> {
      return student.getBirthDate().getTime() > new Date().getTime()
          ? Optional.of(new ValidationError("Student was born in the future."))
          : Optional.empty();
    };
  }

  /**
   * Returns a function that validates a Student's join date.
   */
  public static StudentValidator validateJoinDateFunction() {
    return student -> {
      return student.getJoinDate().getTime() > new Date().getTime()
          ? Optional.of(new ValidationError("Student joined in the future."))
          : Optional.empty();
    };
  }
}

Is this code readable? Does it easily convey to the programmer what it is meant to do? Does it comply with the MVC pattern? How could I change my code to make it look, feel and run better?

Creating validators

I think it's perfectly readable. However, there's a lot of repetition.

To make creating validators simpler and more declarative, you could take it one step further and lift the test/return-Optional logic into a higher-order function:

public class StudentControl {

  public static final StudentValidator namesValidator = validator(
    student -> namePattern.matcher(student.getName()).matches(),
    "Invalid name"
  );

  ...

  private static StudentValidator validator(Predicate<Student> pred, String error) {
    return student -> pred.test(student)
      ? Optional.empty()
      : Optional.of(error);
  }
}

and then if you have other models besides Student, you might genericize the validators.

interface EntityValidator<T> extends Function<T, Optional<ValidationError>> {}

public abstract class EntityControl<T> {
  protected final List<EntityValidator<? super T>> validators = new ArrayList();

  protected EntityValidator<T> validator(Predicate<T> pred, String error) {
    return x -> pred.test(x)
      ? Optional.empty()
      : Optional.of(error);
  }

  // Or, if you want to allow any subclass to create a validator for any type,
  // it could be static:
  protected static <T> EntityValidator<T> validator(...)

  public Optional<List<ValidationError>> validate(T entity) {
    ...
  }
}

This feels a bit cleaner to me as far as separation of concerns, but if you're only ever going to validate Students it's probably overkill.

Other

You might also consider having validate() return an Optional<Set<ValidationError>> instead of the List, if order isn't important. Since validation errors are just strings, you can't effectively do any further handling of them, so I assume they just get piped out to the user somehow?

  Does it comply with the MVC pattern?

I'm not an MVC expert, but: what is the view supposed to be? You haven't included anything that displays Students or errors, receives input, or otherwise does anything with Students, so it's hard to thoroughly assess this.


A LaTeX environment that counts words I am new to TeX and here is my attempt to (ab)use mfirstuc to define an environment that counts the words. I am looking for suggestions on how to improve the implementation.

\documentclass{article}
\usepackage{mfirstuc}
\newcounter{wordcount}
%Make sure we don't accidentally overwrite things with \def and \let
\newcommand\cwaux\relax
\newcommand\oldgmfu\relax
\newcommand\temppar\relax
%
\newtoks\temppar
\def\cwaux#1\par{\capitalisewords{#1}\par}
\newcommand*\myglsmakefirstuc[1]{\stepcounter{wordcount}#1}
\newenvironment{countwords}{%
\setcounter{wordcount}{0}\let\oldgmfu\glsmakefirstuc\let\glsmakefirstuc\myglsmakefirstuc\temppar\everypar\everypar{\cwaux}%
}{%
\everypar\temppar\let\glsmakefirstuc\oldgmfu Word count:\arabic{wordcount}%
}
\begin{document}
\capitalisewords{Outside the environment, \texttt{\textbackslash capitalisewords} works normally.}

\begin{countwords}
One paragraph.

Another paragraph which is longer.

\end{countwords}

\begin{countwords}
The third paragraph.

I am glad to see this works.

\end{countwords}

\capitalisewords{Outside the environment, \texttt{\textbackslash capitalisewords} works normally.}

\end{document}

You can do it in a more robust way, using expl3.

\documentclass{article}
\usepackage{xparse}
\usepackage{mfirstuc}

\ExplSyntaxOn
\NewDocumentEnvironment{countwords}{+b}
 {
  % typeset the contents and add \par at the end
  #1 \par

  % save the contents of the environment in a token list variable
  \tl_set:Nn \l_tmpa_tl { #1 }
  % blank lines generate a \par, change it into a space
  \tl_replace_all:Nnn \l_tmpa_tl { \par } { ~ }
  % split the token list at spaces
  \seq_set_split:NnV \l_tmpa_seq { ~ } \l_tmpa_tl
  % remove blank items
  \seq_remove_all:Nn \l_tmpa_seq { }
  % print the word count
  Word~count:~\int_to_arabic:n { \seq_count:N \l_tmpa_seq }
 }{}
\ExplSyntaxOff

\begin{document}

\capitalisewords{Outside the environment, \texttt{\textbackslash capitalisewords} works normally.}

\begin{countwords}
One paragraph.

Another paragraph which is longer.

\end{countwords}

\begin{countwords}
The third paragraph.

I am glad to see this works.
\end{countwords}

\capitalisewords{Outside the environment, \texttt{\textbackslash capitalisewords} works normally.}

\end{document}


Function that check's file type based on certain keywords I have a function that breaks up a source file and checks each token against a keyword list to determine whether its a particular file type. How can I improve this function? Tokenization is a class that contains a function to break up the source code.

public static boolean validationTypeOfSQL(String sourceFile, Tokenization tokenization,ArrayList<String> uniqueKeywordList) {
    ArrayList<Token> createSourceCodeTokens = tokenization.createSourceCodeTokens(sourceFile);

    for (Token x : createSourceCodeTokens) {
        String smallerTokens[] = tokenization.tokenToSmallerToken(x.getToken());
        for (String smallerTOkenValue : smallerTokens) {
            Boolean result = smallerTOkenValue.contains(",");
            if (result == true) {
                String[] tokenSplit = smallerTOkenValue.split(",");
                for (String ts : tokenSplit) {
                    for (String t : uniqueKeywordList) {
                        if (t.toLowerCase().equals(ts.toLowerCase().replace("(", "").replace(",", "").replace(")", "").replace("\t", ""))) {
                            // System.out.println("This is a mysql file Mysql");
                            return true;
                        }
                    }
                }
            }
            else {
                for (String uniqueKeyword : uniqueKeywordList) {
                    if (uniqueKeyword.toLowerCase().equals(smallerTOkenValue.toLowerCase().replace("(", "").replace(",", "").replace(")", "")))
                    {
                        return true;
                    }
                }
            }
        }

    }
    return false;
}

Regarding your naming:

I'd rename:

validationTypeOfSQL() to isSQL()
createSourceCodeTokens to sourceCodeTokens
createSourceCodeTokens() to sourceCodeTokens()
x to token
tokenToSmallerToken() to smallerTokens()
smallerTOkenValue to smallerToken
result to containsComma (It's better, of course, to remove it completely according to Robert Snyder's answer.)
tokenSplit to splitTokens
ts to st or splitToken
t to k or keyword
uniqueKeyword to keyword

I'd change String smallerTokens[] to String[] .... as you write it 4 lines later anyway.

I' d use replaceAll(String regex, String replacement) with replaceAll("[(,)\t]", "") instead of the replace() chain.


Linked list for interviews I'm trying to learn linked lists so I can answer interview questions. Even though I've seen plenty of tutorials to understand how they supposedly work, I don't know if I am doing a good job because I have no one to get feedback from.

class Node(object):

    def __init__(self, data, next_node = None):
        self.data = data
        self.next = next_node

    def __str__(self):
        return str(self.data)

class SinglyLinkedList(object):

    def __init__(self):
        self.head = None

    def prepend(self, data):
        '''
        Creates a new node and inserts it at the front, making it the head
        of the list
        '''
        self.head = Node(data, next_node=self.head)

    def append(self, data):
        '''
        Creates a new node and inserts it at the very end of the list
        '''
        new_node = Node(data)
        if self.head == None:
            self.head = new_node
        else:
            iter_node = self.head
            while iter_node.next:
                iter_node = iter_node.next
            iter_node.next = new_node

    def insert(self, after_node, data):
        '''
        A new node is created containing the passed data, and then inserted
        right after the node called after_node in the list
        '''
        new_node = Node(data)
        new_node.next = after_node.next
        after_node.next = new_node

    def delete(self, node):
        '''
        Removes the given node from the list. If the node is at the tail already,
        it is set to None. Otherwise it is "de facto" removed by having it mirror the next
        node's data, and then link to that mirror's next node.
        '''
        if node.next == None:
            node = None
        else:
            node.data = node.next.data
            node.next = node.next.next

    def reverse(self):
        '''
        Reverses the entire linked list.
        '''
        if self.head:
            prev = None
            current = self.head
            while current:
                future = current.next
                current.next = prev
                prev = current
                current = future
            self.head = prev

    def __iter__(self):
        '''
        Yields each node in the list over a loop
        '''
        iter_node = self.head
        while iter_node:
            yield iter_node
            iter_node = iter_node.next

    def __str__(self):
        return "[" + ", ".join(str(node) for node in self) + "]"

Bug

SinglyLinkedList.delete called on the tail node doesn't really delete it: the previous node still holds the reference.
Interface

SinglyLinkedList.insert and SinglyLinkedList.delete require a handle to a Node object, which is quite hard to obtain. It seems that the only way is to manually iterate the list breaking on a condition. I recommend prepend, append, insert to return a newly created instance. A find method would also be useful.


Controllers for interacting with a vacation service I have a controller called VacationController; in this controller, I retrieve a list of vacations which will be displayed in a grid.

In the grid, I have the option to create a new vacation through a popup window. Should I create a new controller for this popup, considering that the model is represented by one row of the vacation list?

Those are my controllers:

app.controller('app.VacationController', ['$scope', 'vacationService', function ($scope, vacationService) {

    var vm = this;

    vm.model = {
        hasError: false,
        errorMessage: '',
        hasData:false,
        data: []
    }

    vacationService.get().success(function (vacations) {
        vm.model.hasData = vacations.length !== 0;
        vm.model.data = vacations;
    }).error(function() {
        vm.model.hasError = true;
        vm.model.errorMessage = 'Error';
    });

}]);

app.controller('app.NewVacationController', ['$scope', 'vacationService', function ($scope, vacationService) {

    var vm = this;
    vm.hasError = false;
    vm.error = '';
    vm.data = null;

    vacationService.getById(0).success(function (vacation) {
        vm.data = vacation;
    });

    vm.createNewVacation = function (viewModel) {
        vacationService.post(viewModel.data).success(function () {
            alert('success');
        });
    }
}]);

Should I create a new Controller for Insert?

Yes, and here's why. From a maintenance perspective, this is nightmare waiting to happen. The problem will start to appear when you want your popup used somewhere else independent of the list. However, if you bound your popup to the list controller, you can't move it out because you potentially bound the UI of the popup to some variable in the list controller.

The state of the popup should be independent of the list, thus you need to have a controller (or should I call it "view-model") for it.

As for your code:

I suggest you use the more standard then for promises instead of success and error. This makes it easier to swap out your promise libs/native promises when the time comes.
Use breakpoints to debug code instead of alert (or console or debugger). This prevents you from potentially leaving behind debugging code when you push to production.


Hangman game, written after taking a Python course I have recently finished a course on Python 3 and I really learned a lot, but every time I write a script, it is messy and I often feel like there must be shorter, more efficient ways to write and structure my scripts.

So my question is: How could I rewrite the below script to be shorter and more efficient, and what advice would you guys give me to improve my coding skills in general (maybe a way of thinking about the program that will make it easier to define the structure of what my script should look like...)

I am really enthused about the possibilities that coding bring and I would really like to improve, so don't hesitate to be harsh with your comments ;)

Here below is one of the scripts I wrote. This one is a hangman game: (The word list for the game are being picked from a .txt file that lives in the same directory as my script)

import string
import random

class Main:

    def __init__(self):
        self.place_holder = []
        self.wrong_guesses = 10
        self.guesses = []
        with open('./words.txt', mode='r') as f:
            self.word = random.choice(f.readlines())
            self.word = self.word.strip()
            print(self.word, end='')
            return self.place_holders()

    def place_holders(self):
        # creates the placeholders for the word
        for char in self.word:
            if char != " ":
                self.place_holder.append(' _ ')
            elif char == " ":
                self.place_holder.append('   ')
        print("".join(self.place_holder))  # prints initial placeholder
        return self.is_letter()

    @staticmethod
    def guess():
        # Prompts the user for a guess
        while True:
            guessed_letter = input(str('Give a guess: ')).lower()
            if guessed_letter not in string.ascii_lowercase or len(guessed_letter) > 1:
                print(
                    'You have inputted more than one character or the character entered was not recognized.\nPlease try again.')
            else:
                break
        return guessed_letter

    def is_letter(self):
        # finds out if the letter belongs to the word, and if so, places it on the right placeholder
        guessed_letter = self.guess()
        if guessed_letter in self.word and guessed_letter not in self.guesses:
            for i in range(len(self.word)):
                if guessed_letter == self.word[i]:
                    self.place_holder[i] = f' {self.word[i]} '
        elif guessed_letter in self.guesses:
            print('You already said that..\n')
        else:
            self.wrong_guesses -= 1
            print(f'Sorry, missed.\nYou have {self.wrong_guesses} guesses left.\n')

        self.guesses.append(guessed_letter)
        print("".join(self.place_holder))  # prints the updated placeholder
        return self.is_over()

    def is_over(self):
        # Checks if the players has guessed the full word or if the player ran out of guesses
        if ' _ ' in self.place_holder and self.wrong_guesses > 0:
            self.is_letter()
        elif ' _ ' in self.place_holder and self.wrong_guesses == 0:
            print(f'Sorry, Game Over!\nThe word to guess was: {self.word}')
            self.play_again()
        else:
            self.play_again()

    @staticmethod
    def play_again():
        # Prompts the player if he wants to play again or not
        if input('Do you want to play again? (y/n): ').lower().startswith('y'):
            Main()
        else:
            print('Fair enough.. Thanks for playing!')
            quit()

Main()

Your code structure is one of infinite recursion.  Main() calls placeholder(), which calls is_letter() which calls is_over() which calls is_letter() (recursive), or play_again() which calls Main() (recursive)!  Eventually, you will end up with a Stack Overflow if you play long enough!

What you want is a loop.  Two loops, actually.  Your program should be structured to operate something like:

play_again = True
while play_again:
    # initialize game state
    while not solved and guesses_left:
        # get guess
        # update game state
    play_again = ask_play_again()

No recursive calls are necessary.

You read all lines of the file to choose a random puzzle.  When you play again, you again read all lines in the file.  Perhaps you could read and store all the puzzles, and each time a game is played, you just select one randomly from the list.

Your placeholders are complicated, 3 character entities.  They could just be single characters, and spaces could be added during printing.

self.placeholder = [ "_" if ch != " " else " "  for ch in self.word ]

To print:

print("".join(f" {ch} " for ch in self.placeholder))

Or simply:

print("", "  ".join(self.placeholder))

You should allow for punctuation in the puzzles.  Like spaces, these should be directly shown, not converted to underscores.

 _  _  _  _  _  ’  _     _  _  _  _


Shorty code with range based loops I need an example on writing this code with range based loops:

int pListPet[2] = {34001, 34051};

int pListGlobal[] = {9001, 9002, 9003, 9004, 9005, 9006, 20011, 20091, 20092, 20093, 20094, 20095, 30000};

const char* strMapListGlobal[] = {"string1" , "string2"};

const char* strMapEventOx = "string3";

std::string stringName = CPythonBackground::Instance().GetWarpMapName();

for (int i = 0; i < _countof(strMapListGlobal); i++)
{
    #ifdef CHECK_STRING_3
    if (strMapEventOx == stringName)
    {
        if (0 <= rVictim.GetRace() && rVictim.GetRace() <= 7)
            return FALSE;
    }
    #endif

    if (strMapListGlobal[i] == stringName)
    {
        for (int i = 0; i < _countof(pListGlobal); i++)
        {
            if (rVictim.GetRace() == pListGlobal[i] || pListPet[0] <= rVictim.GetRace() && rVictim.GetRace() <= pListPet[1])
                return FALSE;
        }
    }
}

Do you think this correctly uses std::string? Can this be optimized more or maybe written shorter?

Should I use this?

if (!strcmp(strMapEventOx, stringName))

Range for can be used with iterators, including pointers, as follows

for (auto name: strMapListGlobal)
{
    #ifdef CHECK_STRING_3
    if (strMapEventOx == stringName)
    {
        if (0 <= rVictim.GetRace() && rVictim.GetRace() <= 7)
            return FALSE;
    }
    #endif

    if (name == stringName)
    {
        for (auto p: pListGlobal)
        {
            if (rVictim.GetRace() == p ||
                pListPet[0] <= rVictim.GetRace() &&
                rVictim.GetRace() <= pListPet[1])
                return FALSE;
        }
    }
}

Your program needs further refinement. Upon inspection, we see that the first conditional inside the loop body

if (strMapEventOx == stringName)
{
    if (0 <= rVictim.GetRace() && rVictim.GetRace() <= 7)
            return FALSE;
}

is invariant inside the loop and can be factored out.
That leaves us with the following

for (auto name: strMapListGlobal)
{
    if (name == stringName)
    {
        for (auto p: pListGlobal)
        {
            if (rVictim.GetRace() == p ||
                pListPet[0] <= rVictim.GetRace() &&
                rVictim.GetRace() <= pListPet[1])
                return FALSE;
        }
    }
}

To further refine your program, eliminate raw pointers. Not knowing what your macros do it is difficult to say but you might consider.

auto pets = std::vector<int> { 34001, 34051 };

auto globalPets = {
    9001, 9002, 9003, 9004,
    9005, 9006, 20011,
    20091, 20092, 20093,
    20094, 20095, 30000
};

auto globalEventNames = { "string1" , "string2" };

auto eventOx = "string3";

auto mapName = CPythonBackground::Instance().GetWarpMapName();

#ifdef CHECK_STRING_3
if (eventOx == mapName)
{
    if (0 <= rVictim.GetRace() && rVictim.GetRace() <= 7)
        return FALSE;
}
#endif

for (auto event: globalEventNames)
{
    if (event == mapName)
    {
        for (auto pet: globalPets)
        {
            if (rVictim.GetRace() == pet || pets[0] <= rVictim.GetRace() && rVictim.GetRace() <= pets[1])
                return FALSE;
        }
    }
}

as for strcmp

if (strcmp(strMapEventOx, stringName))

avoid it!

if (strMapEventOx == stringName)

is more readable, more idiomatic, and likely faster.


Inventory system for RPG I'm working on a inventory system for a rpg. I want to be able to compare items, restrict access for specific items(i.e. consumables) for specific inventory slots(i.e. leg, head), summarize items based on their attributes and make it easy to introduce new types of items.

What is a good structure to manage these requirements?

I tried the following, but I have my doubts about it (I shorten it, to make it more readable):

class Attributes{
    public:
        int agility = 0;
};

class Stackable{};

class Inventoryable : Entity{
    public:
        string name = "";
};

class Wearable : Itemable, Attributes{};
class Consumable : Itemable, Stackable{};
class Armor : Wearable{};
class Weapon : Wearable{};
class Cloth : Armor{};
class Pike : Weapon{};

class Item : Inventoryable{
    private:
        Inventoryable inv_;
    public:
        Item(Inventoryable inv_type) : inv_(inv_type) {};
}

int main(void) {
    Item socks = new Item(Cloth);
    socks.name = "Stinky Socks"; //Inventoryable
    socks.agility = 10; //Attributes
}

Depending on how many types of items you will have and how many items will be live at any one time it may pay to use the flyweight pattern. Where each item stack has a pointer to an item type which holds all the behavior, and hold the data that makes the stack unique and/or is used very often (how many items in the stack, id for the sprite to draw, ...).

Making the ItemStack objects fixed size (with a pointer when the you need that extra storage space) will let you make an inventory hold a std::vector<ItemStack> instead of a std::vector<std::unique_ptr<ItemStack>>. The first will have much better performance when iterating over all items in an inventory. Hold by value what you can and use as few pointers as possible for the rest.

I'm not a fan of complex inheritance trees, either you need a virtual function in the base class anyway or you need dynamic_casts all over the place.

Stackable as a tag type is a pretty bad way of making something stackable. You will need to dynamic_cast to find out whether it can stack and the stackable items may each have a different max stacksize that you then need to query. So just create a single virtual size_t get_max_stack() in the base class of the item type.


Finding peak point in an array by using divide and conquer approach It is to find the maximum element in an array which is first increasing and then decreasing. I've tried to write my idea by using divide and conquer approach. Is there any improvable or missing point? My code is following,

def search(arr, low, high):
    if low == high:
        return arr[low]

    mid = low + (high - low) // 2

    if arr[mid] > arr[mid + 1]:
        # if it's peak that a number whose left and right are smaller than it, return it
        if arr[mid] > arr[mid - 1]:
            return arr[mid]
        else:
            return search(arr, low, mid)
    else:
        return search(arr, mid + 1, high)

if __name__ == "__main__":
    arr = [ 70, 80, 9, 6 ]
    arr2 = [60, 70, 72, 74, 88]
    arr3 = [8, 10, 20, 80, 100, 200, 400, 500, 3, 2, 1]
    arr4 = [10, 20, 30, 40, 50]
    arr5 = [1, 3, 50, 10, 9, 7, 6]
    arr6 = [120, 100, 80, 20, 0]

    print(search(arr, 0, len(arr) - 1))
    print(search(arr2, 0, len(arr2) - 1))
    print(search(arr3, 0, len(arr3) - 1))
    print(search(arr4, 0, len(arr4) - 1))
    print(search(arr5, 0, len(arr5) - 1))
    print(search(arr6, 0, len(arr6) - 1))

Output:

80
88
500
50
50
120

The code will crash with an out of bounds exception if given a zero-length array.

You are playing a little loose with the array endpoints.  If you get to mid=0, and it turns out that arr[mid] > arr[mid+1], then you will check arr[mid] > arr[mid-1], which will be reading arr[-1] which is the element at the other end of the array.  Fortunately, if arr[0] > arr[1], then the array must be monotonically decreasing, and the last value will be the smallest, but relying on that is tricky behaviour.

Your code is recursively executing return search(...), Python does not do tail-recursion-optimization.  This could could easily be re-written to use a loop, which would be faster and doesn’t require any additional stack space.  For example, something like:

while low < high:
    mid = low + (high - low) // 2

    if arr[mid-1] < arr[mid] < arr[mid+1]:
        return arr[mid]

    if arr[mid] > arr[mid+1]:
        high = mid
    else:
        low = mid + 1

return arr[low]

You search(mid+1, high). Why don’t you search(low, mid-1)?  Seems asymmetrical.

Minor nit: You algorithm will fail if the array can have equal elements, such as 5, 50, 10, 10, 10, 10, 5.  It will find “10” at the mid-point and on both sides, at which point it will drift to the larger indexes, and eventually declare “10” as the maximum.


Platform-agnostic windowing library I am making a C library that abstracts window creation with support for the new Vulkan API under a unified API;
I have a github repository that you can check out.

main.c

#include "vkwf.h"

int main()
{
    VKWFWindow* window = VKWFCreateWindow("Test Window", 800, 600);

    while (!VKWFWindowShouldClose(window))
    {
        VKWFWindowUpdate(window);
    }

    free(window);

    return 0;
}

The way I am handling this is creating a general vkwf.h file, that includes a list of functions, like this:

vkwf.h

#pragma once

#ifdef __cplusplus
extern "C" {
#endif

#ifdef VKWF_PLATFORM_WINDOWS
#include "win32_window.h"
#elif VKWF_PLATFORM_MACOS
#include "macos_window.h"
#elif VKWF_PLATFORM_LINUX
#include "linux_window.h"
#endif

VKWFWindow* VKWFCreateWindow(const char* title, int width, int height)
{
    return VKWFPlatformCreateWindow(title, width, height);
}

VKWFBool VKWFWindowShouldClose(VKWFWindow* window)
{
    return VKWFPlatformWindowShouldClose(window);
}

void VKWFWindowUpdate(VKWFWindow* window)
{
    VKWFPlatformUpdate(window);
}

void VKWFDestroyWindow(VKWFWindow* window)
{
    VKWFPlatformDestroyWindow(window);
}

#ifdef __cplusplus
}
#endif

All VKWFPlatformX() functions are functions that get defined by the platform_window.h files, like this:

win32_window.h (short)

#define VKWFPlatformCreateWindow(title,width,height) VKWFWin32CreateWindow(title,width,height)
#define VKWFPlatformWindowShouldClose(window) VKWFWin32WindowShouldClose(window)
#define VKWFPlatformUpdate(window) VKWFWin32Update(window)
#define VKWFPlatformDestroyWindow(window) VKWFWin32DestroyWindow(window)

Does this look like a good approach?
Are there any drawbacks, or things that i could improve?

I wouldn't use #define macros. They expose the internal naming convention of your OS dependent functions to the world. Once you send out your library to others [as a shared library], you can never change your internal OS dependent names. The important thing is that public facing names be functions.

For example, if two systems used gcc and created ELF binaries that only called your library functions, could you compile on (e.g.) FreeBSD and that binary would run without rebuild on linux. There are other issues with doing this, so it [probably] isn't practical, but, it's something to think about.

A better way [what I've done when faced with a similar problem] is to put the OS dependent code in a .c and add static to the definitions.

The public functions just call the static ones. The static function names are the same, regardless of platform

The optimizer will either inline the static OS dependent function or will use tail call optimization. So, it's just as fast as macros but a lot cleaner.

Side note: Your naming convention is a bit [MS] Windows centric (i.e. camel hump case). I prefer snake case (e.g. see GTK, etc.). For an example, see the bottom.

vkwh.h:

#pragma once

#ifdef __cplusplus
extern "C" {
#endif

VKWFWindow* VKWFCreateWindow(const char* title, int width, int height);
VKWFBool VKWFWindowShouldClose(VKWFWindow* window);
void VKWFWindowUpdate(VKWFWindow* window);
void VKWFDestroyWindow(VKWFWindow* window);

#ifdef __cplusplus
}
#endif

vkwf.c:

#include "vkwf.h"

#ifdef VKWF_PLATFORM_WINDOWS
#include "win32_window.c"
#elif VKWF_PLATFORM_MACOS
#include "macos_window.c"
#elif VKWF_PLATFORM_LINUX
#include "linux_window.c"
#endif

VKWFWindow* VKWFCreateWindow(const char* title, int width, int height)
{
    return VKWFPlatformCreateWindow(title, width, height);
}

VKWFBool VKWFWindowShouldClose(VKWFWindow* window)
{
    return VKWFPlatformWindowShouldClose(window);
}

void VKWFWindowUpdate(VKWFWindow* window)
{
    VKWFPlatformUpdate(window);
}

void VKWFDestroyWindow(VKWFWindow* window)
{
    VKWFPlatformDestroyWindow(window);
}

win32_window.c:

#include "vkwf.h"

static VKWFWindow*
VKWFPlatformCreateWindow(const char* title, int width, int height)
{
    // ...
}

static VKWFBool
VKWFPlatformWindowShouldClose(VKWFWindow* window)
{
    // ...
}

static void
VKWFPlatformWindowUpdate(VKWFWindow* window)
{
    // ...
}

static void
VKWFPlatformDestroyWindow(VKWFWindow* window)
{
    // ...
}

Here's an example of the snake case for the public functions. Note that since the platform specific functions are static, then can use shorter prefixes:

#include "vkwf.h"

#ifdef VKWF_PLATFORM_WINDOWS
#include "win32_window.c"
#elif VKWF_PLATFORM_MACOS
#include "macos_window.c"
#elif VKWF_PLATFORM_LINUX
#include "linux_window.c"
#endif

VKWFWindow* VKWF_create_window(const char* title, int width, int height)
{
    return platform_create_window(title, width, height);
}

VKWFBool VKWF_window_should_close(VKWFWindow* window)
{
    return platform_window_should_close(window);
}

void VKWF_window_update(VKWFWindow* window)
{
    platform_update(window);
}

void VKWF_destroy_window(VKWFWindow* window)
{
    platform_destroy_window(window);
}


MD5 hash comparison for two folders Using this page for a starting point into comparing MD5 hash values from files in two different folders, I've put something together that outputs either Copied if both files MD5 matches or Check if the MD5 hashes do not match.

I want to find out if there's a better method for accomplishing this.

#Requires -version 4.0
Param
  (
    [parameter(Mandatory=$true,Position=1)][string]$source,
    [parameter(Mandatory=$true,Position=2)][string]$dest
  )
$source = gci $srcfolder -File -Recurse | select FullName,@{Label='Hash'; Expression={(Get-FileHash -Algorithm MD5 $psitem.fullname).hash}}
$dest = gci $destfolder -File -Recurse | select FullName,@{Label='Hash'; Expression={(Get-FileHash -Algorithm MD5 $psitem.fullname).hash}}
#compare $source $dest -Property Hash -PassThru
for ($i=0;$i -lt $dest.Count) {
IF($source.Hash[$i] -match $dest.Hash[$i]){
 "Copied $($source[$i].FullName)"}
Else{
 "Check $($source[$i].FullName)"}
$i++
}

Better is subjective but I will try and improve the code your have here to make it more robust and scalable. I will try to cover areas of improvement or focus and bring it together in the end. A lot of this is error hardening and trying to help remove potentials flaws in the assumptions the code was making.

Pitfalls with non validated paths passed

Hopefully not a nitpick but you are not doing any validation on your paths so it is possible for it to unexpectedly error out if a non-path based string is passed. Looking at about_Functions_Advanced_Parameters we see one option we can you is ValidateScript. The message returned for a failure now is arguably vague but it is a start. Consider one such parameter given the above suggestion.

[parameter(Mandatory=$true,Position=2)]
[ValidateScript({Test-Path -PathType Container $_ })]
[string]$Destination

Now if a bad path is passed the script will error before it starts processing. While we are not avoiding any bad issues really, in this case, this is a good habit to get into.

  Cannot validate argument on parameter 'DifferenceFolder'. The "Test-Path -PathType Container $_ " validation script
  for the argument with value "bagels" did not return a result of True.

Parameter names

You have inconsistent parameter names one for $source and one $dest. The latter one is a short form and the other is not. Also, these variables are common associated with copying. However your not doing that with this function. I am going to side with original author of your code snippet and use more meaningful names. $ReferenceFolder and $DifferenceFolder. Don't worry though. If you really wanted your variable names I would like to propose a compimise and use a parameter alias! This way you could use either and the script works just the same.

[alias("Destination")]
[string]$DifferenceFolder,

Also note worthy is that PowerShell will allow you to use short forms automatically as there is no ambiguity. So you can still use -Dest on the command line as there is only one thing that could mean.

Using Alias in production scripts

Many were consider this a faux-pas as it can obfuscate your code and make people (more ones not familiar with them) not understand what is happening. Testing/Code Golf? Sure use aliases as it can make the code more terse. Production? Use the full names to prevent confusion later if someone else or even you down the road are reading your code.

Using Select after Get-ChildItem

The code is creating a new object array that has the fullname of the file and its hash by use of a calculated property. Get-FileHash contrary to what your linked post says (and the documentation is seems) does accept pipeline input. Also the return properties of the cmdlets are path (which contains the fullpath), hash and algorithm. So there is not need to recreate what is already there. Removing the calculated properties saved some processing time in testing as well.

$referenceFolderHashes = Get-ChildItem $ReferenceFolder -File -Recurse | Get-FileHash -Algorithm $Algorithm | Select Path,Hash

Consider not using MD5

Like CodesInChaos states you should consider not using MD5. There is more likely chance that two different files could generate the same hash with MD5. So SHA256 would be an obvious candidate. Still it is only a chance. I have added a parameter called $Algorithm that will handle this. It is not mandatory and by default is "MD5" for your convenience. We use [ValidateSet] to ensure that you can only choose algorithms supported by Get-FileHash

[parameter(Mandatory=$false)]
[ValidateSet("SHA1", "SHA256", "SHA384", "SHA512", "MACTripleDES", "MD5", "RIPEMD160")]
$Algorithm="MD5"

Your output

I though it odd that you would want to see all the files that copied. It should be easy enough to only see the failures and assume everything else was fine. The part that I was having an issue with was the logic. If the folder contents you are comparing are not in the thousands then Compare-Object should work perfectly fine. So using that we can compare the hash of all files and spit out the ones that do not match.

The part I was having a real issue was that your code was dependent on the directories having the same amount of files. If either directory was missing or had an extra file then your logic would fail since you were comparing element positions and not the contents. This was another reason I opted for Compare-Object

If you do want to see the ones that matches as well I created a [switch] that would help you toggle that off and on again. You can think of it like a boolean. Compare-Object has a parameter for -IncludeEqual we could pass that the value of our switch.

[parameter(Mandatory=$false)]
[switch]$ShowMatches

.....

Compare-Object $referenceFolderHashes $differenceFolderHashes -Property Hash -PassThru -IncludeEqual:$ShowMatches.IsPresent

To continue about the output

So now we have the results of Compare-Object. The pertinent results are under the property SideIndicator which is ==,<= or => which show if an entry is present in only the reference object or difference object. Here I use a calculated property to make that information a little more end-user friendly.

Select-Object Path,@{Name="CopyStatus";Expression={If($_.SideIndicator -eq "=="){"OK"}else{"Check"}}}

Since we are also using the pipeline the way we are the output from this function to something like Export-CSV!

Here is my version of your function.

#Requires -version 4.0
Param(
    [parameter(Mandatory=$true,Position=1)]
    [ValidateScript({Test-Path -PathType Container $_ })]
    [alias("Source")]
    [string]$ReferenceFolder,
    [parameter(Mandatory=$true,Position=2)]
    [alias("Destination")]
    [ValidateScript({Test-Path -PathType Container $_ })]
    [string]$DifferenceFolder,
    [parameter(Mandatory=$false)]
    [ValidateSet("SHA1", "SHA256", "SHA384", "SHA512", "MACTripleDES", "MD5", "RIPEMD160")]
    $Algorithm="MD5",
    [parameter(Mandatory=$false)]
    [switch]$ShowMatches
)

$referenceFolderHashes = Get-ChildItem $ReferenceFolder -File -Recurse | Get-FileHash -Algorithm $Algorithm | Select Path,Hash
$differenceFolderHashes = Get-ChildItem $DifferenceFolder -File -Recurse | Get-FileHash -Algorithm $Algorithm | Select Path,Hash

Compare-Object $referenceFolderHashes $differenceFolderHashes -Property Hash -PassThru -IncludeEqual:$ShowMatches.IsPresent |
    Select-Object Path,@{Name="CopyStatus";Expression={If($_.SideIndicator -eq "=="){"OK"}else{"Check"}}}

Some sample output. I copied a small folder tree and changed a number in one file.

 .\Compare-FilesWithHash.ps1 -Source E:\Temp\csv -Destination 'E:\Temp\csv 2' -ShowCopied

Path                    CopyStatus
----                    ----------
E:\Temp\csv\export.csv  OK
E:\Temp\csv\file1.csv   OK
E:\Temp\csv\file2.csv   OK
E:\Temp\csv\sample1.csv OK
E:\Temp\csv\sample2.csv OK
E:\Temp\csv 2\file.csv  Check
E:\Temp\csv\file.csv    Check

Since the hashed do not match between file.csv then they both are flagged.

I thought about reducing the result set in that case so it only flagged the file once but I opted against it as it would add potentially unnecessary complexity.


"Guess the number Game" in C Just started learning C. I hope my code adheres to best practices and idioms

#include <stdio.h>
#include <ctype.h>
#include <time.h>
#include <stdlib.h>
int main(void)
{
int iRandomNum;
int iGuess;
srand(time(NULL));

iRandomNum = (rand() % 10);
printf("Guess a number between 1 and 10:");
scanf("%d", &iGuess);

// isdigit function expects a character so the guess value has to be ascii value of the digit character
iGuess += 48;
if (isdigit(iGuess)){
    iGuess -=48; // change back to the original value for comparison with random number
    if (iGuess == iRandomNum){
        printf("You Guessed Correctly\n");
     }
    else{
        printf("The correct answer was %d\n",iRandomNum);
        printf("You guessed %d\n",iGuess);
     }
 }
 else{
      printf("This is not a digit!\n");
 }
 }

I have observed a few things that may help you improve your code.

Use consistent formatting

The code as posted has inconsistent indentation which makes it hard to read and understand. Pick a style and apply it consistently.

Check return values for errors

The call to scanf can fail.  You can check the return values to make sure they haven't or your program may crash (or worse) when given malformed input or due to low system resources.  Rigorous error handling is the difference between mostly working versus bug-free software.  You should strive for the latter.

Don't use Hungarian notation

Prefixing every variable with an abbreviation of its type is usually called "Hungarian notation" and it was once popular.  Even then it was a bad idea.  Don't clutter up your source code with that; instead, concentrate on defining meaningful names for each variable and choose types appropriately.  I'd suggest secretNumber and userGuess in this case.

Use a better random number generator

You are currently using

iRandomNum = (rand() % 10);

There are a number of problems with this approach. The most significant problem is that the range this returns (0 through 9 inclusive) does not match what the user is asked to guess ("a number between 1 and 10")  Second, this will generate lower numbers more often than higher ones -- it's not a uniform distribution.  Another problem is that the low order bits of the random number generator are not particularly random, so neither is the result.  On my machine, there's a slight but measurable bias toward 0 with that.  See this answer for details, but I'd recommend changing that to

secretNumber = rand() / (RAND_MAX / 10) + 1;

Think of the user

When someone asks me to "Guess a number between 1 and 10", I usually say \$e\$.  (Yes, I'm afraid I'm that kind of person.)  It seems clear from the context of your program that what you really mean is to guess an integer, and it seems likely that, technically speaking, you don't want numbers solely between 1 and 10 but that you mean to include 1 and 10 among the possible choices.  To more clearly convey that, I'd suggest asking the user to "guess a whole number from 1 to 10 inclusive" but I'm probably too used to being around engineers and scientists.

Check your logic

If I input "e" as my guess, the scanf call will fail and the variable may be uninitialized.


HTML Tidy Sample Program in C++17 This is an exercise to use C++, Cevelop IDE & libtidy rather than the HTML Tidy command line tool. The program just reads from standarg input, processes the contents with HTML Tidy then puts the cleaned up version to standard out. Errors and diagnostics go to standard error. Function tidyit takes a std::string and returns one with error messages going to stderr while main handles the standard I/O.

I am using the most current C++17 with no concerns for older versions. The version of Cevelop IDE is 1.7.1. HTML Tidy provides both a command line tool and a C library, libtidy. The code is based upon the C code in the Sample Program.

I am looking for improvements given it is in C++ rather than the original in C.

// tidyit.cpp
#include <sstream>
#include <iostream>
#include <string>

const std::string tidyit(const std::string a_input);           // Change to auto in cevelop 1.8.

int main(int argc, char **argv) {
   std::stringstream input;
   input << std::cin.rdbuf();
   std::cout << tidyit(input.str());
   }

#include <tidy/tidy.h>
#include <tidy/buffio.h>

bool is_severe_error(int a_code){return (a_code<0);}           // Change to auto in cevelop 1.8.

const std::string tidyit(const std::string a_input) {          // Change to auto in cevelop 1.8.
   TidyBuffer errbuf = {0}, outbuf = {0};
   TidyDoc tdoc = tidyCreate();
   tidyOptSetInt(tdoc, TidyIndentContent, yes);
   tidyOptSetBool(tdoc, TidyXhtmlOut, yes);
   tidyOptSetBool(tdoc, TidyForceOutput,yes);
   int rc{-1};                                                 // Change to auto in cevelop 1.8.
   if (is_severe_error(rc=tidySetErrorBuffer(tdoc, &errbuf))
         || is_severe_error(rc=tidyParseString(tdoc, a_input.c_str()))
         || is_severe_error(rc=tidyCleanAndRepair(tdoc))
         || is_severe_error(rc=tidyRunDiagnostics(tdoc))
         || is_severe_error(rc=tidySaveBuffer(tdoc,&outbuf))){
      std::cerr << "tidyit-A severe error (" << rc << ") occurred.\n";
      }
   else {
      if(errbuf.bp)
         std::cerr << "tidyit-Diagnostics:\n\n" << errbuf.bp << "\n";
      }
   const std::string outstr{(char*)(outbuf.bp)};
   tidyBufFree(&outbuf);
   tidyBufFree(&errbuf);
   tidyRelease(tdoc);
   return outstr;
   }

Cevelop C++ IDE

  Version: 1.7.1-201704211123

Compiler info

  Command: clang++-4.0

  All options: -std=c++1z -O0 -g3 -Wall -c -fmessage-length=0
  -Wno-attributes

Library info

  Libraries: tidy

Updated code available at: CodeReview - HTML Tidy Sample Program in C++17 (pastebin)

Observation.

Though you are compiling this with a C++ compiler and using a couple of C++ classes this is still C code. The style is C and does not reflect the standard idioms of C++.

We refer to code written like this as "C with classes".

Exception Safety

When you see code that looks like:

<Create Resource>

<Do Stuff>

<Destroy Resource>

This is not exception safe. If the code in <Do Stuff> throws an exception then <Destroy Resource> will not be executed. Now you think none of this code throws exceptions and Tidy is a C-library so it will not. But I bet Tidy allows you to register callbacks. These callbacks can throw exceptions and eventually you will add one to the code.

Example:

   TidyDoc tdoc = tidyCreate();
   // Stuff
   tidyRelease(tdoc);

The correct way to do this is to use the RAII idiom. This means creating a class where the constructor allocates the resource and the destructor deallocates the resource.

Since the destructor of an object is automatically called when an object goes out of scope (this includes when the stack is unwound by an exception being thrown) this makes resource allocation/deallocation exception safe.

class TidyDocCPP
{
    TidyDoc tdoc;
    public:
        TidyDocCPP() {
            tdoc = tidyCreate();
        }
        ~TidyDocCPP() {
            tidyRelease(tdoc);
        }
        // This is used to convert an object back to the C form
        // so it can be used in the normal C tidy functions.
        operator TidyDoc() const {
            return tdoc;
        }

        // Need to do this to satisfy rule of 5
        // Don't worry about this section yet.
        TidyDocCPP(TidyDocCPP const&)            = delete;
        TidyDocCPP(TidyDocCPP&&)                 = delete;
        TidyDocCPP& operator=(TidyDocCPP const&) = delete;
        TidyDocCPP& operator=(TidyDocCPP&&)      = delete;
};

Now it can be used like this:

{
    TidyDocCPP    doc;
    tidyOptSetInt(doc, TidyIndentContent, yes); // doc is auto converted
}
// doc goes out of scope here.
// This calls the destructor and releases the resource.

Design

You read the input into a string. Then pass this string to tidyit().

The first issue I have with this is that you pass the string by value. This basically makes a copy of the whole string. Given that modern web pages are huge this is a terrible idea. The main page of stackoverflow.com is 250K.

You should pass immutable strings by const reference (this prevents the copy but still allows full read access).

But even reading the data into a string is incorrect. You have a stream of data (the input) you should use Tidy stream reading capabilities.

Rather than using

 tidyParseString(tdoc, a_input.c_str()

You should use:

 tidyParseStdin(doc);

If you want to be more generic:

 tidyParseSource(doc, inputSource);
 // You will need to build a wrapper aournd TidyInputSource
 // http://tidy.sourceforge.net/docs/api/group__IO.html#g86fcc3c86bd63b26a559938bc38d34bb

Vertical Space

Add vertical space to make your code readable. The current way it is all jammed together makes it highly unreadable.

Brace Alignment

Use one of the standard brae alignment strategies. Yours does not match any standard I have seen.

// K&R Style.  Very common
if () {
}

// Allman.     Very common
if ()
{
}

What you are using seems to be a mix of K&R and Ratliff Style

// Ratliff     Very uncommon
if () {
    }

I have never seen it outside this wikipedia page: https://en.wikipedia.org/wiki/Indent_style

Pick one and use it consistently.


Applying chain responsibility with IFilter I have an interface IFilter and more types of filters: Filter1, Filter2, Filter3; that I want to call one after the other and if the result of one is empty, then I want to stop the process. For this I have thought of:

class FiltersSequence
{
private:
    std::vector< IFilter > m_filters;

public:
    FiltersSequence();
    ~FiltersSequence();

    /**/
    ObjectToFilter execute(const ObjectToFilter& objectToFilterIn);
};

With the implementations:

FiltersSequence::FiltersSequence()
{
    m_filters.push_back(Filter3());
    m_filters.push_back(Filter2());
    m_filters.push_back(Filter1());
}

FiltersSequence::~FiltersSequence() {}

ObjectToFilter FiltersSequence::execute(const ObjectToFilter& objectToFilterIn)
{
    ObjectToFilter filterResult = objectToFilterIn;

    for (std::size_t i = 0; i < m_filters.size(); i++)
    {
        filterResult = m_filters[i].execute();
        if (filterResult.isEmpty())
        {
            return filterResult;
        }
    }

    return filterResult;
}

It should be vector of pointers, because the filters are not all the same size; so I have changed to:

std::vector< std::shared_ptr< IFilter > > m_filters;

and

m_filters.push_back(std::shared_ptr< IFilter >(new FilterX()));

I thought of something like having a map< std::string, IFilter > for being sure that the sequence is in the order that I want. What do you think of this? Is it good enough? Or this is just making it more complicated and I need just to pay attention at push_back. BTW, is this going to call them in right order (1, 2, 3)?

I thought of something like having a map< std::string, IFilter > for
  being sure that the sequence is in the order that I want. What do you
  think of this?

std::map for such simple case should be avoided. std::map would be more appropriate if you are not sure that your IFilter objects could be inserted in any order by client code and you do have lots of entry. Of course we should measure our program performance by both(.i.e. std::vector or std::map

  Is it good enough? Or this is just making it more complicated and I
  need just to pay attention at push_back.

Yes, it look ok. We should always use more simpler/efficient data structure(std::vector) as our default container unless we have valid reason to do otherwise.

BTW, is this going to call them in right order (1, 2, 3)?

No, It would be in order of (3,2,1) as std::vector would push_back. We can see that documentation clearly mention like this about push_back

Appends the given element value to the end of the container.

[Filter3()][Filter2()][Filter1()]
   0           1          2