PHP class which preserves line break by splitting the line into as many line break as it have Can anyone here please review my PHP code which preserves line break by splitting the line into as many line break as it have?

class PerserveLineBreak
{
    //Preserves line break in HTML element content while stripping by splitting the html element content into two items
    protected $text;
    protected $pattern;
    protected $preservedContent;

    function __construct($text, $pattern="
-")
    {
        $this->text = $text;
        $this->pattern = $pattern;
        $this->init();
    }

    function init(){
            $this->preservedContent = $this->perserveLineBreak($this->text,$this->pattern);
    }

    function preparePattern($pattern)
    {
        //adding delimiter and use preg_quote
        $pattern = "#" . $pattern . "#";
        return preg_quote($pattern);
    }

    function getPreservedContent(){
        return $this->preservedContent;
    }

    function perserveLineBreak($content, $searchPattern)
    {
        //looks in for
 by default
        return $this->stripListOfString((preg_split($this->preparePattern($searchPattern), $content)));
    }

    function stripListOfString($listString)
    {
        //strips the splitted html element
        $strippedList = array();
        foreach ($listString as $item) {
            array_push($strippedList, trim(strip_tags($item)));
        }
        return $strippedList;
    }

}

I am brand new to PHP programming language and interested in improving my coding standard and OOP design?

Please do review my code and give me constructive feedback?

After about 10 minutes of reading your code and testing it out, I think I figured out what your class is supposed to, which is not a good sign for such a small class.

To me it seems that the goal of your class is to strip all the tags from a string containing HTML code, without losing the
 tags, by replacing these tags with newlines. However, when running your code I did not see this behaviour at all.

I ran your code using the default $pattern parameter and the following HTML as input:

"Hello
World!

How are you doing?

Great!
"

and the result (using var_dump()) from your getPreservedContent() method was:

array(1) { [0]=> string(35) "HelloWorld!How are you doing?Great!" }

Which I am not sure what to do with. Why is it in an array? Where are the newlines? They do not seem to be preserved at all!

I am assuming that the desired result for this HTML string would be:

string(67) "Hello
World!
How are you doing?

Great!
"

Based on my observations of the incorrect functionality of your class, design flaws and untidiness I will propose a redesign of your class and explain why I made the changes that I made.

Redesign

First of all, your class seems to be implemented specifically for the use of keeping HTML line breaks. This means that you have two choices here. Either choose to create a class for a more general use case, or make your class very specific to the HTML domain. I would chose for the latter here.

The name of your class does not describe what the intent of your class is. It only states that it preserves line breaks, not that is also strips the HTML tags from the string. I would suggest a name like:

class HtmlTagStripper { ... }

Which arguably might not be a very good name either, but I think it is a step up from where we were, as it clearly states the intent of the class.

This class could then have two public functions, one for regular stripping of tags and one for stripping the tags whilst preserving the newlines.

public function strip($html_text) { ... }

public function stripWithPreservedLineBreaks($html_text) { ... }

As you can see, I have chosen to require the user to pass the HTML text into the function instead of into the constructor of the class. This makes the class stateless and allows for reuse of the class without having to create a new instance.

Furthermore, we do not want the user to be able to pass in a pattern to the constructor either, since this class only works with HTML and therefore only works with
 tags. Combining these two adjustments, we no longer need to specify a constructor at all!

To match the
 tags, we need a regular expression, which you already presented in your code via the $pattern parameter. I will implement this regular expression as a constant on the class. The tricky part is that
 tags can be written in any of the following ways:
,
,
,
,
 and many more slight variations. So we need to make the regular expression a bit more robust.

I found such a regex in this StackOverflow question. Implementing this regex as a constant would look like this:

class HtmlTagStripper {
    const BR_TAG_REGEX = '/
]+)*[\/]?>/gi';

    ...
}

Now that we have this out of the way, lets start with the implementation of the regular strip() function.

public function strip($html_text) {
    return strip_tags($html_text);
}

Now lets implement the stripWithPreservedLineBreaks() function.

public function stripWithPreservedLineBreaks($html_text) {
    $html_text = $this->convertLineBreakTagsToNewlines($html_text);

    return $this->strip($html_text);
}

private function convertLineBreakTagsToNewlines($html_text) {
    return preg_replace(self::BR_TAG_REGEX, PHP_EOL, $html_text);
}

There are two things to take note of here. First, we have delegated the responsibility of replacing the
 tags with newlines to a separate function with a very clear name, to improve readability. It is important to specify that this method is private and cannot be used outside of your class, as it encapsulates the internal workings of the class. Second, we delegate the actual stripping of the tags to the basic strip() function that we wrote earlier.

The class as a whole now looks like this:

class HtmlTagStripper {
    const BR_TAG_REGEX = '/
]+)*[\/]?>/gi';

    public function stripWithPreservedLineBreaks($html_text) {
        $html_text = $this->convertLineBreakTagsToNewlines($html_text);

        return $this->strip($html_text);
    }

    private function convertLineBreakTagsToNewlines($html_text) {
        return preg_replace(self::BR_TAG_REGEX, PHP_EOL, $html_text);
    }

    public function strip($html_text) {
        return strip_tags($html_text);
    }
}

What to take away from this

You did not specify any visibility modifiers for your functions. Always make clear which functions are meant to be exposed (public) and which are internal (private/protected).
Naming is extremely important in code readability. Name your classes, methods, variables and parameters appropriately.
Always test your code!

Testing

Something I haven't touched on in this review is testing. You should always test your code to ensure/prove that it works the way you intended. Use a unit testing library such as phpunit to test many different inputs to your functions and verify that they are correct.

An example test:

class HtmlTagStripperTest {
    public function testSimpleBrTagIsReplacedWithNewline() {
        $html_tag_stripper = new HtmlTagStripper();
        $stripped_html = $html_tag_stripper->stripWithPreservedLineBreaks("
");
        $this->assertEquals(PHP_EOL, $stripped_html);
    }

    public function testComplexBrTagIsReplacedWithNewline() {
        $html_tag_stripper = new HtmlTagStripper();
        $stripped_html = $html_tag_stripper->stripWithPreservedLineBreaks("
");
        $this->assertEquals(PHP_EOL, $stripped_html);
    }
}

These two tests verify that different variations of the
 tag are both replaced with a newline character. Of course you will need to test many more variations as well as test bigger HTML strings and so on.


Use predefined vs. custom enums to return appropriate message from service layer In my project I used N Layer Architecture.

In the service layer I return  an enum  for each action, for example for Add action I have an enum like this :

  public enum AddStatus
{
    AddSuccessful,
    Faild,
    Exists
}

And for Edit I have this :

    public enum EditStatus
{
    Successful,
    Faild,
    NotFound,
    Exists
}

But sometimes I need a custom enum like this one :

    public enum AddAssetFunctionStatus
{
    AddSuccessful,
    Faild,
    Exists,
    ValueIsLess,
    ItIsNotStatistical
}

and with custom enum I can show the appropriate message to the end user.

Another way I use, which I think is good, is to pass an out string param to each action, like :

   AddStatus Add(AddAccessDeviceViewModel accessDeviceViewModel,out string message);

And in the controller I have :

 switch (result)
        {
            case AddStatus.AddSuccessful:
                _uow.SaveChanges();
                this.AddNotification(Messages.SaveSuccessfull, NotificationType.Success);
                return RedirectToAction(MVC.Admin.AccessDevice.Index());
            case AddStatus.Faild:
                this.AddNotification(Messages.SaveFailed, NotificationType.Warning);
                break;
            case AddStatus.Exists:
                this.AddNotification(Messages.DataExists, NotificationType.Info);
                break;

        }

Which one is better?

Use custom enum for each action that I need.
pass an out param to the action that I need.

I would not recommend using this pattern, personally. What you're effectively trying to do is signal error state, where 0 is null state and therefore success, similar to the way that you might do it with the Win32 API. The designers of c# and the .NET framework explicitly sought to avoid this pattern (and deem it to be an anti pattern). I think it works great for OS level work, and clearly does for both Windows and Linux, but it can be very cumbersome.

The "c#" method of doing this is by using exceptions. For a method called SaveChanges, the return type is usually either void or bool. I personally recommend void, because bool doesn't give any context for what caused the operation to fail, and is really just a slim version using an enumeration.

For the remainder of cases, there is USUALLY a built in exception type that is appropriate. InvalidOperationException can be used to state that you can't save the changes because the entity already exists, for instance. In a Web API, you can return something in one of the error ranges for this case.

If you really look at the exception model,  I believe you'll find that it does provide a decent solution to this problem for you. It liberates you from having to build complex enumerations, and mapping them across layers. Your application is then free to focus on interactions with the layer it communicates with, and you don't need to build extra services across each tier to describe what happened. Ideally, this means easier long term maintenance as well, because changing any part of the implementation does not result in needing to update the ENTIRE implementation.


Efficient File Searching During an interview I was asked to consider the following:

  You are trying to find an instance of an error code within a number of log files within a directory. The goal is to count the number of times that the error code appears within the specified files.

For the written function, it was to be passed the following arguments (the directory and general file naming convention were known):

  [TimeStamp]: String

  [ErrorCode]: String

Afterward I went home and coded up a basic implementation of a single file search algorithm with, I believe, O(1) search time after the files are parsed. Does this implementation make sense, and is it correct in terms of Java 8?

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.*;
import java.util.stream.Collectors;
/**
 * Created by Brendan on 5/13/2016.
 *
 * A quick implementation of an interview File Search algorithm.
 * Parses the file into a hash map so that occurrences of a phrase
 * can be quickly looked up.
 */
public class FileSearch {
    public static void main(String[] args) {
        Map<String, Long> wordMap = new HashMap<>();
        try {
            wordMap = populateWordMap("C:\\Projects\\logs\\app-log.log");
        } catch(IOException e) { }

        System.out.println("Total unique words found: " + wordMap.size());
        System.out.println("Most popular word: " + findMostPopularWord(wordMap));
    }

    private static String findMostPopularWord(Map<String, Long> wordMap) {
        return wordMap.entrySet()
                        .stream()
                        .max(Map.Entry.comparingByValue())
                        .get().getKey();
    }

    private static Map<String, Long> populateWordMap(String filePath) throws IOException {
        return Files.lines(Paths.get(filePath))
                .map((wholeLine) -> wholeLine.split(" "))
                .map((lineArray) -> mapWordsInLine(lineArray))
                .map(Map::entrySet)
                .flatMap(Collection::stream)
                .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue, (m1, m2) -> m1 + m2));
    }

    private static Map<String, Long> mapWordsInLine(String[] lineArray) {
        return Arrays.stream(lineArray)
                    .collect(Collectors.groupingBy(String::toString, Collectors.counting()));
    }
}

Comments about your current code first:

Closing the Stream

The method Files.lines(path) returns a Stream<String> over the lines of the file. This stream should be closed because it encapsulates a Reader of the file. The documentation mentions that:

  If timely disposal of file system resources is required, the try-with-resources construct should be used to ensure that the stream's close method is invoked after the stream operations are completed.

Therefore, the method populateWordMap should be using a try-with-resources:

try (Stream<String> stream = Files.lines(Paths.get(filePath))) {
    // ...
}

Allocating unused objects

You currently allocate the wordMap with a new hash map, and then store the result of invoking populateWordMap in it.

Map<String, Long> wordMap = new HashMap<>();
try {
    wordMap = populateWordMap("C:\\Outils\\eclipse-mars\\workspace\\test\\text.txt");

The consequence is that the first hash map that was created for nothing: it is unused and will be garbage collected. Best not to create it in the first place.

Empty catch blocks

try {
    // something
} catch(IOException e) { }

is generally not a good idea: you are completely swallowing the exception that could be thrown and giving no information. You could log the exception to tell what went wrong; you could also rethrow it; or you could even let the main method throw it, since there is really nothing you can do at this level when something wrong happens.

Handling empty Optional

Your findMostPopularWord method is searching for the word appearing the most times. It works by correctly using the max operation. But then, you have:

.max(Map.Entry.comparingByValue()).get().getKey();

To get the key of the entry having the max value. The problem is that when the Stream is empty, max will return an empty Optional and the explicit call to .get() will throw an exception. This raises the more general question of what you want to happen where the scanned file is empty (and contains no words). Among the possible solutions: just return null or throw a custom exception. For those two solutions, you can use .orElse(null) or orElseThrow(...), instead of get.

Looping over all files in a directory

I'll note that your current solution only works by counting word occurences in a single file when the original task was to do it for all files in a directory. For that enhancement, you can use Files.list(path) which returns a Stream<Path> of all the files under the given path.

The current flow of your code is:

Loop over each line of the file
Split each line in an array of words
Replace each array of words with a map containing the occurrences of each word
Replace each of those map with another map containing the occurrences of each entry's value.

This is not very efficient: you are converting each line into a map, only to disregard them later and create another map out of all those maps.

The problem is that you're thinking in terms of "line in a file" when the actual task is to count space separated words. For this task, you can use a Scanner:

private static Map<String, Long> populateWordMap(String filePath) throws IOException {
    try (Scanner scanner = new Scanner(Paths.get(filePath))) {
        Iterable<String> iterable = () -> scanner;
        return StreamSupport.stream(iterable.spliterator(), false)
                     .collect(Collectors.groupingBy(Function.identity(), Collectors.counting()));
    }
}

A scanner tokenize by default around single spaces. We can then convert the scanner (which is an Iterator) to a Stream and group by each word while counting their occurences.


User-input prime number generator What do you consider the readability of this program?
Do you think it's easy to understand what the variables and functions do just by reading their names?
Would you prefer more, less, or this amount of comments?
Do the comments make it easier or harder for you to understand the program?
Do you like a program to have comments (in general)?

# This program generates prime numbers

# Introduces user
def welcome():
    print("""
         Welcome to the prime number generator!

         This program genarates all prime numbers starting from zero
         to an upper limit. You´ll decide the upper limit.

         Have fun :)
         """)

# User inputs a natural number that will be the upper limit for checking primenumbers
def limit(question):
    while True:
        try:
            limit = int(input(question))
        except:
            limit = ""
        if limit:
            return limit
            break

# Checks if the number is a prime number. If it is, the number will be returned. If it is not, then None will be returned
def is_it_prime(current_number, prime_number):
    y = 0
    while True:
        ratio = current_number / prime_number[y]
        y += 1
        if ratio == int(ratio):
            break
        if y == len(prime_number):
            return current_number

# Adds prime number to list
def ad_prime(prime_number, it_is_prime):
    a = len(prime_number)
    prime_temp = (a+1)*[None]
    for i in range(a):
        prime_temp[i] = prime_number[i]
        if i+1 == a:
            prime_temp[i+1] = it_is_prime
    return prime_temp

# Main program
upper_limit_question = "Type the upper limit and press enter. It has to be a natural number expressed in digits: "
lower_limit = 2
prime_number = [2]
welcome()
upper_limit = limit(upper_limit_question)
while lower_limit < upper_limit:
    lower_limit +=1
    it_is_prime = is_it_prime(lower_limit, prime_number)
    if it_is_prime != None:
        prime_number = ad_prime(prime_number, it_is_prime)
print("\n" +str(prime_number), "\n\nThanks for generating prime numbers :)")

It is easy to get a general understanding of what your code does.  You did a good job separating the code into functions.  It could be made more readable in a few ways though, mostly by doing things in a simpler way.  For example, your add_prime function (which appends a number to the end of a list) could be written in one line (I'll get to how later).

I recommend reading the Python tutorial.  By the time you get to chapter 4, you'll notice a lot of thing you would have done differently. :)

The variable names are good and make it easy to follow what you're doing, but the are a couple of names that are a bit confusing.  In particular, prime_number should be prime_numbers or simply primes because it is a list of numbers.  Also, limit isn't very descriptive at all.  The difference between the limit function and the upper_limit variable is that one gets the value from the user, and the other stores it.  limit should be get_limit or get_limit_from_user.

I like the way you comment.  The amount of comments is good and they improve readability.  Keep commenting like that.  I have one note, though:  When you're writing a comment that describes what a whole function or a whole program does, use a docstring.  This makes it clearer to other programmers what you commenting about, and makes it possible to access the comment in the code.

Here is what an improved version of the code could look like (comments starting with ## are part of this review and don't belong to the program):

"""Generate prime numbers."""    ## This is a docstring.

WELCOME_TEXT = """Welcome to the prime number generator!

This program generates all prime numbers starting from zero to an upper limit.
You'll decide the upper limit.

Have fun :)
"""

LIMIT_PROMPT = ("Type the upper limit and press enter. It has to be a natural "
                "number expressed in digits: ")

ALL_CAPS_NAMES are a convention meaning a value should be treated as constant.  In general, you should set constant values at the start of your program.  In this case, this also has the advantage that the WELCOME_TEXT won't be indented.

The two strings in LIMIT_PROMPT will automatically be concatenated.  This way, you can write a one-line string in two lines of code.

def welcome():
    """Introduce user."""    ## Also a docstring.  If your function definition starts
    print(WELCOME_TEXT)      ## with a string, the string will be made a docstring.

def get_limit():
    """Let user input a an upper limit for checking prime numbers.

    The input must be a natural number."""

    while True:
        try:
            return int(input(LIMIT_PROMPT))
        except ValueError:
            pass

I made a few changes to the get_limit function.  You should always catch only specific exceptions (ValueError in this case.) Also, I think it's better not to mix strings and numbers in one variable.

def is_prime(current_number, prime_numbers):
    """Check whether the number is a prime number.

    prime_numbers must contain all prime numbers lower than current_number."""

    for prime in prime_numbers:
        if current_number % prime == 0:    ## (current_number % prime) is the remainder
            return False                   ## when current_number is divided by prime.
    return True

You can loop over a list can be done easier this way.  Making is_prime return a boolean will make things easier for you in the main part of the program.

def main():
    welcome()
    upper_limit = get_limit()
    prime_numbers = []
    for number in range(2, upper_limit+1):
        if is_prime(number, prime_numbers):
            prime_numbers.append(number)    ## This adds number to the end of prime_numbers,
                                            ## so you don't need add_prime anymore.
    print("\n", prime_numbers, "\n\nThanks for generating prime numbers :)")

main()

I put the main part of the program into a main function (this is a useful convention).  Again, you can simplify the loop, this time using the range function.  For simple loops, there is usually a way of using a very readable for loop instead of a while loop.  I also changed the name lower_limit to number, which fits better in my opinion.

If you have any questions, feel free to ask them.  Welcome to codereview. :)


Android TCP server that maps data to an object and sends it to a service This is a code for an android thread. This is a tcp server and gets data from socket, maps it to an object and sends that object back to the main service. If there is anything I'm doing wrong or if something can be done better, I would really appreciate comments.

public class SocketManager extends Thread {

    Handler mHandlerThread;
    private final int MAX_MESSAGE_LENGTH = 2048;
    private final int PORT = 2500;
    private ServerSocket serverSocket = null;
    private Socket socket = null;
    private DataInputStream in = null;
    private DataOutputStream out = null;
    private GPSFields gps;
    private ObjectMapper mObjectMapper;
    private final String APPNAME = "My App";

    public SocketManager(Handler mHandlerThread) {
        this.mHandlerThread = mHandlerThread;
    }

    @Override
    public void run() {

        int messageLength = 0;
        gps = new GPSFields();
        byte buffer[] = new byte[MAX_MESSAGE_LENGTH];
        mObjectMapper = new ObjectMapper();
        mObjectMapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);

        try {
            openSocket();
        } catch(Exception e) {
            Log.d(APPNAME, e.toString());
        }

        while(true) {
            try {
                if(socket.isClosed()) {
                    openSocket();
                }
                else {
                    messageLength = in.readShort();
                    if (messageLength > 0 && messageLength < MAX_MESSAGE_LENGTH) {
                        in.read(buffer, 0, messageLength);
                        gps = mObjectMapper.readValue(new String(buffer), GPSFields.class);
                        if (gps != null) {
                            out.write(1);
                            Message message = new Message();
                            message.obj = gps;
                            mHandlerThread.sendMessage(message);
                            Log.d(APPNAME, "Received Latitude: " + gps.getLatitude() + " Longitude: " + gps.getLongitude());
                        } else {
                            out.write(0);
                        }
                    } else {
                        out.write(0);
                    }
                }
            } catch (Exception e) {
                closeSocket();
                Log.d(APPNAME, e.toString());
            }
        }
    }

    private void closeSocket() {
        try {
            socket.close();
            serverSocket.close();
        }catch (Exception e){
            Log.d(APPNAME, e.toString());
        }
    }

    private void openSocket() {
        try {
            serverSocket = new ServerSocket(PORT);
            socket = new Socket();
            socket = serverSocket.accept();
            in = new DataInputStream(socket.getInputStream());
            out = new DataOutputStream(socket.getOutputStream());
        }catch (Exception e){
            Log.d(APPNAME, e.toString());
        }
    }
}

When messageLength happens to be too large the code just loops around to the next messageLength = in.readShort();. However, the message body is still there, and readShort will pull the short from it. Chances are, it would be interpreted as a valid message length, and the stream gets irrecoverably out of sync.

You must discard the entire message by reading it.
DataInputStream.read() hey return less bytes than requested. Yet again, the stream gets out of sync. You must read the message body in the loop, until it is consumed entirely.
Closing the server socket every time a data socket fails seems drastic. There is nothing wrong with the server socket, and could be reused. It may need to be reopened only if accept fails.
I recommend to move openSocket call to the exception clause:

    while (True) {
        try {
            messageLength = in.readShort();
            ....
        } catch(Exception e) {
            closeSocket();
            Log.d(APPNAME, e.toString());
            openSocket();
        }

One nesting level down.
You may want to be more precise about exceptions. It is very well possible that the exception has nothing to do with the socket (e.g. may sendMessage throw? was it a keyboard interrupt? ObjectMapper throw? something else? It is impossible to tell from the given code). In such cases different recovery is necessary, but the socket should likely remain intact.


Random Password Generator based on user input This is my second program written in python. It basically creates a password made up of numbers, capital and lower case letters with the option to include special characters and dashes. I created a simpler project and I decided to build upon it to create version 2. Since I am still new, I would like to receive advice and any thoughts on how to improve my code as well as a programmer. Such as how random this password is, any syntax errors, or any ways to simplify my code. I commented as best as I could. Any thoughts would be appreciated.

#version 2 introduces option to include special characters and
the option for the password to be separated into
#sections by a dash as well as a more secure and better created
password which has no certain pattern to it
#as the previous version did

#loop goes through steps until user decides to quit
# 1. ask for user input if special characters can be added and set
     to var response
# 2. come up with random number between 1 and 3, set = to var
     typevalue if special is no
#    come up with random number between 1 and 4, set = to var
     typevalue if special is yes
# 3. assign randint between 0 and 25 to lowercase and uppercase
     var, assign randint between 0-9 to number var,
#    assign randint between 0-9 to character var
# 4. if type=0 number value is added to objects list
#    if type=1 upper[uppercase] is added to objects list
#    if type=2 lower[lowercase] is added to objects list
#    if response is yes and only yes, characters[character] is
     added to objects list

from random import randint

upper =
["A","B","C","D","E","F","G","H","I","J","K","L","M",
 "N","O","P","Q","R","S","T","U","V","W","X","Y","Z"]
lower = ["a","b","c","d","e","f","g","h","i","j","k","l","m",
         "n","o","p","q","r","s","t","u","v","w","x","y","z"]
characters = ["!","@","#","$","%","^","&","*","_","+"]

objects = []

def RunFun(a,b):
    typevalue = randint(a, b)
    lowercase = randint(0, 25)
    uppercase = randint(0, 25)
    number = randint(0, 9)
    character = randint(0, 9)
    if typevalue == 1:
        objects.append(upper[uppercase])
    elif typevalue == 2:
        objects.append(number)
    elif typevalue == 3:
        objects.append(lower[lowercase])
    elif typevalue == 4:
        objects.append(characters[character])

def RunType2(c,d):
    first = c
    second = d
    for i in range(16):
        RunFun(first, second)
    Function1()

def Function1():
    dashes = raw_input("Would you like it to be seperated into"
    four sections of four characters divided by a dash?
    Y for yes N for no: ")
    if dashes == "Y" or dashes == "y":
        #print "dashes %s" % (objects)
        print "%s%s%s%s-%s%s%s%s-%s%s%s%s-%s%s%s%s" % (
            objects[0], objects[1], objects[2], objects[3],
            objects[4], objects[5], objects[6], objects[7],
            objects[8], objects[9], objects[10], objects[11],
            objects[12], objects[13], objects[14], objects[15])
    else:
        #print "no dashes %s" % (objects)
        print "%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s" % (
            objects[0], objects[1], objects[2], objects[3],
            objects[4], objects[5], objects[6], objects[7],
            objects[8], objects[9], objects[10], objects[11],
            objects[12], objects[13], objects[14], objects[15])

#-------------foundation-----------
while True:
    #requests user input which determines if special characters
    #are included
    answer = raw_input("Can it have special characters? Y for yes
    N for no. X to exit: ")

    if answer == "Y" or answer == "y":
        RunType2(1,4) #hands over value 1 and 4 to include
                      #characters in the creation
    elif answer == "N" or answer == "n":
        RunType2(1,3) #hands over values 1 and 3, opts out 4
                      #which is special characters
    elif answer == "x" or answer == "X":
        exit()
    else:
        print "Invalid Input."
            del objects[:]

At the top of the module use triple quotes to document things instead of using comments. At the top level it will also act as a docstring for your module.
Use random.choice on the iterable instead of getting a random int first and then using it get an from iterable.
if answer == "Y" or answer == "y" etc can be replaced with if answer.lower() == "y".
Use better variables names, for example a and b in FunRun(a, b) doesn't make any sense.
Use new style string formatting instead of the old sprintf based syntax. Or better join str.join in your case if you want to merge bunch of items.
Your code has too many PEP 8 issues as per PEP8 online tool. You should check PEP 8 and use extensions like flake8, pylint etc to catch those.


Read a config file into an array I just wanted to know if this looks like a good way of reading a config file. Is this code good? Is it clean and easy to follow?

<?
    /* class to parse a txt config file
     *
     * Boolean like string values (on/off, yes/no, true/false) will return true/false
     * Numberic string values will return real numeric values
     *
     * Ingore error on input.
     *
     * useage: $cp = new config_parse();
     *         $config_parse->parse_file($input_file);
     *
     * $input_file - the file to be parsed. If a full path is not specified, will assume the file is in the same path as the script calling this class.
     * returns an array of config values in their correct type or false if an error is encountered.
     *
     */

    class config_parse {

        var $config_array;
        var $config_file;
        var $comment_marker = "#";
        var $bool_true_array = array("on", "yes", "true");
        var $bool_false_array = array("off", "no", "false");

        function config_parse() {

            $this->config_array = array();
            $this->config_file = "";

        }

        // parse the config file into the config array
        public function parse_file($input_file) {

            if ($input_file=="") {
                return false;
            }

            // locate the config file
            if (file_exists($input_file)) {
                $this->config_file = $input_file;
            } elseif (file_exists(dirname(__FILE__) . $input_file)) {
                $this->config_file = dirname(__FILE__) . $input_file;
            } else {
                return false;
            }

            $this->do_parse();
            return $this->config_array;

        }

        // read the lines of the file
        private function do_parse() {
            $handle = fopen($this->config_file, "r");
            if ($handle) {
                while (($line = fgets($handle)) !== false) {
                    $this->process_config_line($line);
                }
                fclose($handle);
            } else {
                return false;
            }
        }

        // remove blank lines and comments
        private function process_config_line($line) {
            $line = trim($line);

            // blank
            if ($line=="") {
                return;

            // comment
            } elseif (substr($line,0,1)==$this->comment_marker) {
                return;

            // actual value
            } else {
                $this->process_name_value($line);
            }
        }

        // process a line with an actual name/value pair in it.
        private function process_name_value($line) {

            $line_split = explode("=",$line);
            $name = trim($line_split[0]);
            $value = trim($line_split[1]);

            $this->config_array[$name]=$this->process_value($value);
        }

        private function process_value($value) {

            if (in_array($value,$this->bool_true_array)) return true;
            if (in_array($value,$this->bool_false_array)) return false;
            if (is_numeric($value)) return $value+0;
            return $value;

        }
    }
    ?>

This code looks pretty good, but you seem to have largely reinvented parse_ini_file($filename, false, INI_SCANNER_TYPED).  All you need to add is the support for the search path.

do_parse() is inconsistent about returning a status code, and parse_file() fails to check it.

You should call explode("=", $line, 2), so that any = character in the value will be interpreted literally.


Simple class to handle Json I created a simple class which serialises and deserialises Json and C# objects. How is it looking?

JsonHandler:

class JsonHandler
{
    public T GetObjFromJson<T>(string filePath)
    {
        T retObj = default(T);

        try
        {
            string jsonStr = string.Empty;
            if (File.Exists(filePath))
            {
                byte[] bytes = File.ReadAllBytes(filePath);
                jsonStr = Encoding.Default.GetString(bytes);
                if (!string.IsNullOrWhiteSpace(jsonStr))
                    retObj = JsonConvert.DeserializeObject<T>(jsonStr);
            }
        }
        catch (Exception ex)
        {
            string message = ex.Message;
        }

        return retObj;
    }

    public bool WriteObjAsJson(string filePath, object obj)
    {
        bool successfulWrite = false;

        try
        {
            string jsonString = JsonConvert.SerializeObject(obj);
            byte[] bytes = Encoding.Default.GetBytes(jsonString);
            File.WriteAllBytes(filePath, bytes);
            successfulWrite = true;
        }
        catch (Exception ex)
        {
            string message = ex.Message;
        }

        return successfulWrite;
    }
}

The main things I'm concerned about is the reading all bytes. I guessed that is the most bulletproof solution? Ideally I'd be using this as part of a PCL in a mobile application (Xamarin) in the near future.

class Program
{
    static void Main(string[] args)
    {
        string filePath = @"C:\Users\me\somewhere\textFile.txt";
        var d = new DummyClass("Danny");
        var handler = new JsonHandler();
        bool successfulWrite = handler.WriteObjAsJson(filePath, d);
        Console.WriteLine(successfulWrite);
        DummyClass returnObj = handler.GetObjFromJson<DummyClass>(filePath);
        if (returnObj != null)
            Console.WriteLine($"Name: {returnObj.Name}");
    }
}

class DummyClass
{
    private string _name;
    public string Name { get { return _name; } set { _name = value; } }

    public DummyClass(string name)
    {
        Name = name;
    }
}

I am only using the DummyClass for some quick testing purposes. Not so concerned with the above two classes, and I only included them to show an example of how the JsonHandler class will be used.

Here are some suggestions

be consistent: jsonStr vs. jsonString
be consistent: GetObjFromJson vs WriteObjAsJson (maybe Read & Write instead)
use guard clauses instead of nesting
avoid temporary variables like successfulWrite and retObj

Based on the suggestions, here is one alternative:

static class JsonHandler // class could be static
{
    public static T GetObjFromJson<T>(string filePath)
    {
        // use a guard clause to reduce nesting
        if (File.Exists(filePath)) return default(T);

        try
        {
            var jsonStr = File.ReadAllText(filePath);

            // guard clause again
            if(string.IsNullOrWhiteSpace(jsonStr)) return default(T);

            // we could use null coalesing operator ?? here
            return JsonConvert.DeserializeObject<T>(jsonStr) ?? default(T);
        }
        catch (Exception ex)
        {
            // do proper exception handling
        }

        return default(T);
    }

    public static bool WriteObjAsJson(string filePath, object obj)
    {
        try
        {
            string jsonStr = JsonConvert.SerializeObject(obj);
            File.WriteAllText(filePath, jsonStr);
        }
        catch (Exception ex)
        {
            // do proper handling
            return false;
        }

        return true;
    }
}

Also you should think about exception handling.

if the method returns default(T), maybe reflect that in the method name
if the method throws on failure, maybe wrap the exception into a custom one
if the method fails, return null to signal that
use the Try* pattern, look at bool int.TryParse(string,out int)

Also, in the DummyClass you can use Auto-Implemented Properties public string Name { get; set; }


Getting table cell contents in a row and displaying them as a concatenated tooltip on each cell I have a JavaScript code that gets the inner text content of certain table cells in a row and displays them as a tooltip on mouseover. However, I am a novice at JavaScript and I do not know how to make the code more efficient and/or secure from exploits.

<!DOCTYPE html>

<meta charset='utf-8' />
<title>Test page</title>

Name
Population (2000)
Population (2005)

San Antonio
3,420
3,572

Green Valley
1,409
1,222

As far as making the code "secure from exploits", that might be tough to do with JavaScript, unless you take tactics like minification/obfuscation, etc. The suggestions below should allow the code to be more robust.

Naming

elem, elem1 and elem2 aren't very descriptive. Better names might be nameElement, populationElement1, populationElement2, etc.

Additionally, the variable in the for loop (i.e. i) is not declared using var so it is put into the global namespace.

Allow number of population columns to be dynamic

Currently the code only allows two columns of populations, and hard-coded to years 2000 and 2005. The table headers could have a class added:

Name
Population (2000)
Population (2005)

Then those population headers can be fetched from the DOM using document.getElementsByClassName() and stored in a variable:

var populationHeaders = document.getElementsByClassName('populationHeader');

Then the function placeTitle() can iterate over those titles and add the title attributes to the elements using the iterator variable, eliminating the need for elem1 and elem2:

for (var k = 0; k < populationHeaders.length; k++) {
    var populationCell = row.children[k+1];
    populationCell.title = nameElement.textContent + '\n' + populationHeaders[k].textContent+':' + populationCell.textContent;
}

Setting the titles on mouseover?

Why call placeTitle() when the use mouses over each row? This can lead to calling the function hundreds of times when it really only needs to be called once. So instead of the following:

for (i = 0; i < trLen; i++) {
    hoverRow[i].setAttribute("onmouseover","javascript: placeTitle(this);");
}

It would be better to just call placeTitle() once per row (instead of on every mouse over event):

for (i = 0; i < trLen; i++) {
    placeTitle(hoverRow[i]);
}

Or this could be done with the function approach - using Array.forEach():

hoverRow.forEach(placeTitle);

Duplicate function

Your code has two functions called placeTitle(). The first gets overwritten by the second, thus making it useless. Make sure your function names are unique.

var hoverRow = document.querySelectorAll("#table1 tbody tr"),
  trLen = hoverRow.length;

var place = document.getElementById('placeName');
var populationHeaders  = document.getElementsByClassName('populationHeader');

function placeTitle(row) {
  nameElement= row.children[0];
  nameElement.title = nameElement.textContent;
  for (var k = 0; k < populationHeaders.length; k++) {
    var populationCell = row.children[k+1];
    populationCell.title = nameElement.textContent + '\n' + populationHeaders [k].textContent+':' + populationCell.textContent;
  }
}

for (i = 0; i < trLen; i++) {
  placeTitle(hoverRow[i]);
}

      Name
      Population (2000)
      Population (2005)

      San Antonio
      3,420
      3,572

      Green Valley
      1,409
      1,222

Functional approach to iterating over the headers

In the snippet below, the headers are iterated over using Array.forEach(). That way there is no need to index into the array of population headers. document.getElementsByClassName() returns a HTMLCollection interface (different than an array) so the spread syntax (i.e. ...) is used to put the elements into an array.

var hoverRow = document.querySelectorAll("#table1 tbody tr"),
  trLen = hoverRow.length;

var place = document.getElementById('placeName');
var populationHeaders = document.getElementsByClassName('populationHeader');

function placeTitle(row) {
  nameElement= row.children[0];
  nameElement.title = nameElement.textContent;
  [...populationHeaders].forEach(function(populationHeader, index) {
    var populationCell = row.children[index + 1];
    populationCell.title = nameElement.textContent + '\n' + populationHeader.textContent + ':' + populationCell.textContent;
  });
}
hoverRow.forEach(placeTitle);

      Name
      Population (2000)
      Population (2005)

      San Antonio
      3,420
      3,572

      Green Valley
      1,409
      1,222

For more tips on interacting with the DOM via Javascript, check out this article. I know it is a few years old but still relevant*.

*at the time of writing


Search module methods Constructive criticism is required for the 2 methods below.  I'm trying to develop better OO skills.

 // Set up scanner to allow for searching of modules
public static String searchModule() {
    Scanner scan;
    System.out.print("Search module code: ");
    scan = new Scanner(System.in);
    String searchModule = scan.nextLine().trim();
    return searchModule;
}

// Search modules and return all students enrolled on it
public static void moduleStudentSearch(Set<Module> modules, Set<Student> students) {
    String searchModule = searchModule();

    for (Module module : modules) {
        if (searchModule.equalsIgnoreCase(module.getName())) {
            Iterator it = students.iterator();
            while (it.hasNext()) {
                Student student = (Student) it.next();
                if (student.getModules().contains(module)) {
                    System.out.printf("%s ", student.getName());
                }
            }
        }
    }
}

I'd rename the searchModule method to readModuleName since it doesn't search, it just reads the name of the name of the searched module.
Instead of the iterators use the new foreach loop (as you already use it for the modules):

for (Student student: students) {
        ...
}

If you use iterators set the type parameter which makes the casting unnecessary:

final Iterator<Student> it = students.iterator(); // type parameter here
while (it.hasNext()) {
    Student student = it.next(); // no casting here
    ...
}

Declare variables on the first use:

Scanner scan = new Scanner(System.in);

Don't forget to close the Scanner That was a bad advice, since it closes System.in too. See also: Close a Scanner linked to System.in
Split up the moduleStudentSearch method, it does four things:

read the searched module name
search the module by name
search student by module
print the results

Each function should have a separate method:

public static void moduleStudentSearch(final Set<Module> modules, final Set<Student> students) {

    final String searchedModuleName = readModuleName();
    final Module module = searchModule(searchedModuleName, modules);
    if (module == null) {
        printNoModuleFound(searchedModuleName);
        return;
    }
    final Set<Student> studentsWithModule = getStudentsWithModule(students, module);
    printStudents(studentsWithModule);
}

private static void printNoModuleFound(String searchedModuleName) {
    System.out.println("No module found with name: " + searchedModuleName);
}

public static String readModuleName() {
    System.out.print("Search module code: ");
    final Scanner scan = new Scanner(System.in);
    try {
        final String searchModule = scan.nextLine().trim();
        return searchModule;
    } finally {
        scan.close();
    }
}

// you could return a list/set here if there are more than one results
public static Module searchModule(final String searchedModuleName, final Set<Module> modules) {
    for (final Module module: modules) {
        final String moduleName = module.getName();
        if (searchedModuleName.equalsIgnoreCase(moduleName)) {
            return module;
        }
    }
    return null;
}

public static Set<Student> getStudentsWithModule(final Set<Student> students, final Module module) {
    final Set<Student> result = new LinkedHashSet<Student>();
    for (final Student student: students) {
        if (student.hasModule(module)) {
            result.add(student);
        }
    }
    return result;
}

private static void printStudents(final Set<Student> students) {
    for (final Student student: students) {
        System.out.print(student.getName());
        System.out.print(" ");
    }
}

I'd create a StudentManager and a ModuleManager class. searchModule should be in the ModuleManager and it should use its internal Set<Module> modules field instead of the method parameter. The same is true for the getStudentsWithModule method: it should be inside the StudentManager class and it should use its internal Set<Student> students field instead of the method parameter. It results high cohesion.

Once you have these classes, remove the static modifier of the methods.
Check your input: check for nulls, empty Strings etc. and throw proper exceptions (usually NullPointerException or IllegalArgumentException) with a helpful message:

public static Set<Student> getStudentsWithModule(final Set<Student> students, final Module module) {
    if (students == null ) {
        throw new NullPointerException("students cannot be null");
    }
    if (module == null) {
        throw new NullPointerException("module cannot be null");
    }
    ...
}


Implementation of a binary heap I've read a little about binary heaps/priority queues and decided to try my hand at making my own implementation of one. I'm not the most experienced programmer and I would be really grateful if you guys could take a look at my heap class and tell me if it's a good implementation.

What can I improve here? Any feedback is welcome.

public class Heap<T extends Comparable<? super T>> {

    private T[] array = (T[])new Comparable[10];
    private int size = 0;

    public void insert(T data) {
        if(size+1 > array.length) expandArray();

        array[size++] = data;
        int pos = size-1;
        T temp;

        while(pos != 0 && array[pos].compareTo(array[pos/2]) < 0) {
            temp = array[pos/2];
            array[pos/2] = array[pos];
            array[pos] = temp;
            pos /= 2;
        }
    }

    public T deleteMin() {
        T min = array[0];

        array[0] = array[size-1];
        array[size-1] = null;
        size--;
        int pos = 0;
        T temp;
        boolean done = false;

        while(pos*2+1 < size && !done) {
            int minChild = pos*2+1;
            if(array[minChild].compareTo(array[pos*2+2]) > 0) minChild = pos*2+2;

            if(array[pos].compareTo(array[minChild]) > 0) {
                temp = array[minChild];
                array[minChild] = array[pos];
                array[pos] = temp;
                pos = minChild;
            }
            else done = true;
        }

        return min;
    }

    private void expandArray() {
        T[] newArray = (T[])new Comparable[array.length*2];

        for(int i = 0; i < array.length; i++)
            newArray[i] = array[i];

        array = newArray;
    }
}

The API of your class: in my opinion, there should be one more method: getMin that returns the minimum element but not removes it. The only way to get it now without deleting is to call deleteMin and then re-insert it into the heap, which is inconvenient and inefficient.
There is a bug in the deleteMin method. The fact that pos * 2 + 1 is less than size does not imply that pos * 2 + 2. It can lead to a NullPointerException. I have written a simple unit test that exposes the problem:

import org.junit.Test;

import static org.junit.Assert.*;

public class HeapTest {

    @Test
    public void testDelete() throws Exception {
        Heap<String> aHeap = new Heap<String>();
        aHeap.insert("a");
        aHeap.insert("b");
        aHeap.insert("c");
        assertEquals("a", aHeap.deleteMin());
    }
}

The result:

java.lang.NullPointerException
    at java.lang.String.compareTo(String.java:1142)
    at java.lang.String.compareTo(String.java:111)
    at Heap.deleteMin(Heap.java:33)
    at HeapTest.testDelete(HeapTest.java:13)

You should always test your code in a systematic manner. Unit tests are a good way to do it. They will help you to find and fix bugs and to make sure that it works properly(but not to prove its correctness, of course). They will increase maintainability and reliability of your code. You should write unit tests for all public methods. Do not forget to test edge cases(deleting an element from an empty heap and so on).
You expand an array when necessary, but you never shrink it. It can lead to a big memory overhead. For example, if we insert a lot elements into the heap and then remove most of them, the array will not shrink, consuming much more memory than necessary. You can fix it by either adding a shrinkArray method or by using a List instead of an array here. An ArrayList can handle all required operations for you. So I would use it, unless you decided not to use a List on purpose as an exercise. If it is the case, I'd still recommend creating another class that implements the List interface(or provides a similar functionality) to decouple a heap from a dynamic array. It will make your code more readable, flexible(what if you decide to change the implementation of a dynamic array one day?) and testable.
Code style:

It is conventional to surround binary operators with whitespaces. There should also a whitespace between an if, for or while keyword and an opening parenthesis. Here is a small piece of your code rewritten according to the style conventions:

while (pos != 0 && array[pos].compareTo(array[pos / 2]) < 0) {
    temp = array[pos / 2];
    array[pos / 2] = array[pos];
    array[pos] = temp;
    pos /= 2;
}

Blank lines: a lot of blank lines inside one method don't look good, in my opinion. They can be used for separating different logical blocks of code from each other(but in this case it might be better to create two separate methods instead), but they are definitely redundant in small methods(which is the case here).
In general, it is possible to make your code more concise by using the Java standard library. For instance, you can use an Arrays.copyOf method instead of the expandArray. But I believe that it is fine here because it is an exercise, not a production code(otherwise, you could have just used a PriorityQueue, but that's obviously not what you want in this case).

You should write doc comments for all public classes and methods.

Summary:

To improve you code, you can do the following:

Write unit-tests.
Write doc comments.
Adhere to the code style conventions.
Design classes in such a way that one class is responsible for one concern.


Rendering function for total player points I have a render function:

render: function(playerId) {
    this.getTotalPoints(playerId);
    // and some other code after this;
}

This render function may be executed with or without a playerId.

This is the getTotalPoints function:

getTotalPoints: function(playerId) {
     if (playerId) {
         this.allplayers[playerId].totalPoints = this.calculatePoints(this.allplayers[playerId].cards);
     } else {
         this.allplayers.forEach(function(element, index) {
             element.totalPoints = this.calculatePoints(element.cards);
         }.bind(this));
     }
 }

And the third function that actually calculates the points:

calculatePoints: function(cards) {
    points = 0;

    for (var i = 0; i < cards.length; i++) {
        points += cards[i].points;
    };

    return points;
}

I am repeating myself in getTotalPoints, where I have a call to this.calculatePoints - one for a single player and then one for all the players, depending on whether the playerId is set or not.

Is there any chance I can avoid this and simplify the code?

Split code of getTotalPoints function into two steps:

Save list of players into temp variable. If playerId is specified, then create array with only one element;
Call calculatePoints in a foreach.

Other suggestions:

Put var before points variable in calculatePoints function to move it from global to local scope;
Rename getTotalPoints function to updateTotalPoints;
Depending on the rest of your code it may be possible to pass array of players into render function. And move playerId check out of it.


C# System.Numerics.Vector Clamp The software that I develop uses large floating-point arrays up to the maximum size that can be allocated in C#. I have a large number of algorithms such as convolutions and filters that get executed over those large arrays. I am currently updating as many algorithms as possible to fully threaded and vectorized.

By utilizing the System.Numerics.Vector<T> methods, I am seeing typically a 300%+ performance improvement in many of the algorithms on computers equipped with AVX (where Vector<float>.Count returns 4) and a 600%+ performance improvement in many of the algorithms on computers equipped with AVX2 (where Vector<float>.Count returns 8).

NET Standard 2.1 System.Numerics.Vector<T> here:

https://docs.microsoft.com/en-us/dotnet/api/system.numerics.vector-1?view=netstandard-2.1

One of the functions that I require on a number of the algorithms is to Clamp the array element value to a bounds Minimum or Maximum value after performing some mathematical operation on it. That is of course really easy to do with the single-threaded and multi-threaded algorithms that use standard arithmetic operations.

The issue that I am having is that System.Numerics.Vector<T> doesn't include any kind of Clamp method (Vector2, 3, 4 do). So, for example, if I loop over a large array, modifying the array in Vector<float>.Count chunks, I need to clamp each vector result to a min and/or max bounds prior to writing that vector-sized chunk back to the array.

I tried doing the Clamp in a loop on the array chunk data after the Vector operations, but the performance is abysmal. It is as slow or slower than simply doing the algorithm without vectorization.

Is there any way that I can conceivably improve the performance of this Clamp method?

This is some typical code of how I tried clamping. I fill the vector with a chunk of the array, perform some vector math, write the chunk back to the array, this is all nice and speedy, but then Clamping the array chunk in a loop after just kills the vectorization performance advantage.

int length = array.Length;

int floatcount = System.Numerics.Vector<float>.Count;

for (int i = 0; i < length; i += floatcount)
{
    System.Numerics.Vector<float> arrayvector = new System.Numerics.Vector<float>(array, i);

    arrayvector = System.Numerics.Vector.Multiply<float>(arrayvector, 2.0f);
    // There may be different or multiple vector operations in here.

    arrayvector.CopyTo(array, i);

    // This is how I tried clamping the array data after the vector operation:
    for (int j = 0; j < floatcount; j++)
    {
        if (array[i + j] > maximimum) { array[i + j] = maximimum; }
    }

}

I'm probably being myopic and missing something really simple. That's what months of 16-hour programming days gets you. ;) Thanks for any insight.

Have you tried something like this:

Create the following vector:

  System.Numerics.Vector<float> maxima = new System.Numerics.Vector<float>(maximimum);

Then after the multiplication call:

arrayvector = System.Numerics.Vector.Min(arrayvector, maxima);

Here you may have to create an new vector instead of reassigning to arrayvector?

So all in all it ends up like:

  int length = array.Length;

  int floatcount = System.Numerics.Vector<float>.Count;
  System.Numerics.Vector<float> maxima = new System.Numerics.Vector<float>(maximimum);

  for (int i = 0; i < length; i += floatcount)
  {
    System.Numerics.Vector<float> arrayvector = new System.Numerics.Vector<float>(array, i);

    arrayvector = System.Numerics.Vector.Multiply(arrayvector, 2.0f);
    arrayvector = System.Numerics.Vector.Min(arrayvector, maxima);
    // There may be different or multiple vector operations in here.

    arrayvector.CopyTo(array, i);

  }

Disclaimer: I haven't tested the above, so don't hang me if it's not an improvement :-)


Python - First project user system This code is supposed to be the "user" system for my app. This will ask you what you want to do...
If you want to add a user, remove a user or check the list of saved users.
With this I learned how to open and edit text files. And my goal doing projects is keep on learning and maybe someday I will be able to work or professionally use Python.
This code is working at the moment, but I wanted to know if is too basic or too complicated with some more professional eyes on it (please consider my 3 weeks knowledge).
snames = list()
f_n = (open('names.txt')).read()
print("Welcome to NAME.app")
##################    USER   LOGG IN        #######################
while True:
    name = input("\n - Insert name to logg in \n - ADD to save new user \n - LIST to see saved users \n - REMOVE to delete a user \n - EXIT to finish \n ...")
    if name.lower() == "add":
        n_input = input("Name:")
        with open('names.txt', 'a') as f:
            f.write(n_input + '\n')
            f.close()
        continue

    elif name.lower() == "list":
        with open('names.txt') as f:
            print(f.read().splitlines())

    elif name in f_n:
        print("Logged as", name.upper())
        nxt = input('Welcome, press enter to continue \n')
        if nxt == '':
            break

    elif name.lower() == 'remove':
        rem = input("Insert user name to remove \n ...")
        with open('names.txt', 'r+') as f:
            l=f.readlines()
            l=[z for z in l if rem not in z]
        with open('names.txt', 'w') as f:
            f.writelines(l)

    elif name.lower() == "exit":
        exit()

First thing I notice is that this...
nxt = input('Welcome, press enter to continue \n')
if nxt == '':
    break

...could just be:
input('Welcome, press enter to continue')

It doesn't seem like you're using nxt for anything else so there's no need to store a value for it, especially if you just want the user to hit enter.

Next, you never do f_n.close() which can cause some issues with the file (e.g., it doesn't save or something weird happens with the garbage collector system, etc.).
You might want to replace...
elif name.lower() == "exit":
    exit()

...with:
elif name.lower() == "exit":
    f_n.close()
    exit()

The same should be done with the file in the first elif. Something I also noticed about this is that f_n is the same file as f. You shouldn't need to keep opening it; just use the f_n variable.

Instead of...
if name.lower() == "add":
    n_input = input("Name:")
    with open('names.txt', 'a') as f:
        f.write(n_input + '\n')
        f.close()
    continue

...with:
if name.lower() == "add":
    n_input = input("Name:")
    with open('names.txt', 'a') as f:
        f.write(n_input + '\n')
        f.close()

You don't need to say continue because the rest of the while loop is just elifs.

In terms of overall code style, mostly good, but there are a few things you might want to keep in mind:

Put spaces around equals signs for readability (e.g., use n = 10 instead of n=10).
It's spelled "log" not "logg."
I don't see a usage of snames, so is it really necessary?
The first comment in your code looks rather odd. Maybe just say # USER LOGIN , use a simple block comment, or, if you have to:

##############
# USER LOGIN #
##############

Hope this helps! If I think of something else, I'll edit it in.


Sorting a list of numbers in increasing order Here's a program I wrote to sort a list of numbers in increasing order (without using inbuilt sort function).

(define (sort-list l)
  (define first-element (if (not (null? l)) (car l) 0))
  (cond ((null? l) (quote ()))
        (else (cons (find-shortest l first-element) (sort-list (remove-member l (find-shortest l first-element)))))))

(define find-shortest
    (lambda (tl b)
      (cond ((null? tl) b)
            ((< (car tl) b) (set! b (car tl)) (find-shortest (cdr tl) b))
            (else (find-shortest (cdr tl) b)))))

(define remove-member
    (lambda (tl2 a)
      (cond ((null? tl2) (quote ()))
            ((= a (car tl2)) (cdr tl2))
            (else (cons (car tl2) (remove-member (cdr tl2) a))))))

It works, but I know it's very badly written. It has way too many recursions than what's necessary. Can someone suggest ways to make this code better?

What you've written is called "selection sort", and it works fine. It's not the best sort, but it's not bad, either.  It's one of a class called "n^2" sorts, because it generally runs in time proportional to the square of the length of the list.

Selection sort is a somewhat painful sort to write in a recursive style, because removing an element from a list is not so natural.

You can make it look a smidgen more natural if you rebuild the list on the way back out after removing it. However, the second pass over the list (as you've written it) does not change the asymptotic running time; it's still "order n-squared".

One note: you gain nothing by mutating "b" in the body of "find-shortest." Instead, replace

(set! b (car tl)) (find-shortest (cdr tl) b)

with

(find-shortest (cdr tl) (car tl))

... to make your code shorter and cleaner.

Also, If you were in my class, I would ask you for purpose statements and test cases. :)


Simple react app to fetch and display data This is my first react app and it was for a job interview challenge. The app is really simple and it works, but although I don't think I'm gonna get it, could I have some feedback about what is wrong and how could I improve it?

***I already submitted this, so is not cheating, I just need to know how can I be better

The way requests are being made is correct?
Should planetas object be populated this way? Is there a better approach? (thats how I managed to make it work)
planetas.films was taking too long to be loaded and giving me a map is undefined problem, is my conditional render bad?
And anything else you find to be wrong

service:

import axios from 'axios'

const PLANETS_URL = 'https://swapi.co/api/planets'
const FILMS_URL = 'https://swapi.co/api/films'

const GetPlanets = async () => {
    let planetas = {}

    await axios.get(`${PLANETS_URL}`)
        .then((p) => {

            planetas = p.data.results

            return axios.get(`${FILMS_URL}`)
        }).then((f) => {

        planetas.forEach((pv, i) => {
            pv.films.forEach((pf, j) => {
                f.data.results.forEach((el, k) => {
                    if(pf === el.url){
                        pv.films[j] = el
                    }
                })
            })
        })
        })
        .catch((e) => {
            console.error(e);
        })

    return planetas
}

export default GetPlanets

component:

import React, { useState, useEffect } from 'react'
import GetPlanets from '../services'
import { Loader, Container, Name, Info, Button } from '../components'

export default function Planetas () {

  const [ counter, setCounter ] = useState(1)
  const [ planetas, setPlanetas ] = useState([])
  const [ toShow, setToShow ] = useState({})

  useEffect( () => {

    const fetchPlanetas = async () => {
      const planetas = await GetPlanets()

      setPlanetas(planetas)
      setToShow(planetas[0])
    };
    fetchPlanetas()
  }, [])

  const handleClick = () => {
    if(counter === (planetas.length - 1)){
      setCounter(0)
      setToShow(planetas[counter])
    } else {
      setCounter(counter + 1)
      setToShow(planetas[counter])
    }
  }

  if(typeof toShow.films !== 'undefined') {
    return (
      <Container>
        <Name>{toShow.name}</Name>
        <Info>

Population: {toShow.population}

Climate: {toShow.climate}

Terrain: {toShow.terrain}

        Featured in Films:

        {toShow.films.map((film, key) => (

{film.title}

           ))}

        </Info>
        <Button onClick={handleClick}>Next</Button>
      </Container>
     )
  } else {
    return(
      <Loader>Loading...</Loader>
    )
  }
}

Requests could be handled better by first fetching planets and then fetching films, instead of waiting for all data to fetched before render. I would prefer to make a service call to get planets and then fetch films asynchronously after that.
As I mentioned above, I think it's better to get planets and then fetch films asynchronously.
Since you're looping through all planets and films, I expect it to take long time. In addition, you're waiting for films to be fetched all and then you display planet information. Also, this approach would not be working correct if there was more than one page of films response.
You can fetch planet and keep a current index value to show current planet information. And you can fetch featured films of that planet after planet info is rendered. To implement a better structured code, you can also use reducers.

I've implemented two versions, first version is without reducers and second one is with reducers. You can take a look at to get some idea and compare both solutions.

Without reducer: https://codesandbox.io/embed/swapiplanetsfilms-simple-wdtoh

With reducer: https://codesandbox.io/embed/swapiplanetsfilms-reducer-yg44l


A recursive function to reverse string Write a recursive version of the function reverse(s), which reverses the string s in place.

I'm studying K&R and wrote a code for the question.
void reverse(char s[], int n)   // The seconde argument should be always 1.
{               // The function should be always called as reverse(s, 1);
    int temp;
    int len;

    len = strlen(s);
    if ((len + 1) / 2 > n)
        reverse(s, n + 1);

    temp = s[n-1];
    s[n-1] = s[(len - 1) - (n-1)];
    s[(len - 1) - (n-1)] = temp;
}

The second argument n says reverse() is currently called at n-th times. The function works as if it sets an axis of symmetry at s[(strlen(s) + 1) / 2](since integer division is truncated, it's not always symmetric by the axis but the function behaves as if truncation doesn't happen and it is exactly an symmetric axis) and accordingly change s[n - 1] with s[(strlen(s) - 1) - (n - 1)] and vice versa. The function stops its recursive call when the index of the axis is larger than n.
I saw other solutions(this and this). I think they used nice idea for the question. But I wonder how you think about mine. Thanks for reading!

To be honest, this is a bad way to solve the problem - recursion with strlen() is not a good idea. There was a recent discovery that Rockstar Games had their loading screens in GTA5 extended by several minutes due to their JSON parser calling strlen() on a string repeatedly.
Instead, when handling strings in a recursive manner that requires knowledge of the length of the string, one should pass that length alongside the pointer.
On the topic of length, you're storing the length of the string in a variable with type int rather than the size_t that strlen() returns. There are two problems with this - the first is that int is signed and size_t is unsigned, and the second is that size_t is often larger than int, especially with modern 64-bit systems defining the former as uint64_t and the latter as int32_t.
Lastly, you could avoid a lot of mental overhead by using two pointers instead - one to the beginning of the string, and one to the end of the string. You would then stop the recursion when the beginning is no longer before the end.


Searching for words in a dictionary I'm using an open source program called MElt which lemmatize (give the lemma example:giving-->give) of words. MElt works on Linux and it's programmed in Perl and Python. So far it's working good but it take way too much time to give the results. I looked into the code and located the loop responsible for this:

while (<LEFFF>) {
  chomp;
  s/ /_/g;
#  s/(\S)-(\S)/\1_-_\2/g;
  /^(.*?)\t(.*?)\t(.*?)(\t|$)/ || next;
  $form = $1; $cats = $2; $lemma = $3;
  #print "$form \n";
  #print "$cats \n";
  #print "$lemma \n";
  if ($lower_case_lemmas) {
    $lemma = lc($lemma);
  }
  if ($it_mapping) {
    next if ($form =~ /^.+'$/);
    next if ($form eq "dato" && $lemma eq "datare"); # bourrin
    next if ($form eq "stato" && $lemma eq "stare"); # bourrin
    next if ($form eq "stata" && $lemma eq "stare"); # bourrin
    next if ($form eq "parti" && $lemma eq "parto"); # bourrin
    if ($cats =~ /^(parentf|parento|poncts|ponctw)$/) {$cats = "PUNCT"}
    if ($cats =~ /^(PRO)$/) {$cats = "PRON"}
    if ($cats =~ /^(ARTPRE)$/) {$cats = "PREDET"}
    if ($cats =~ /^(VER|ASP|AUX|CAU)$/) {$cats = "VERB"}
    if ($cats =~ /^(CON)$/) {$cats = "CONJ"}
    if ($cats =~ /^(PRE)$/) {$cats = "PREP"}
    if ($cats =~ /^(DET)$/) {$cats = "ADJ"}
    if ($cats =~ /^(WH)$/) {$cats = "PRON|CONJ"}
    next if ($form =~ /^(una|la|le|gli|agli|ai|al|alla|alle|col|dagli|dai|dal|dalla|dalle|degli|dei|del|della|delle|dello|nei|nel|nella|nelle|nello|sul|sulla)$/ && $cats eq "ART");
    next if ($form =~ /^quest[aei]$/ && $cats eq "ADJ");
    next if ($form =~ /^quest[aei]$/ && $cats eq "PRON");
    next if ($form =~ /^quell[aei]$/ && $cats eq "ADJ");
    next if ($form =~ /^quell[aei]$/ && $cats eq "PRON");
    next if ($form =~ /^ad$/ && $cats eq "PREP");
    next if ($form =~ /^[oe]d$/ && $cats eq "CONJ");
  }
  $qmlemma = quotemeta ($lemma);
  for $cat (split /\|/, $cats) {
    if (defined ($cat_form2lemma{$cat}) && defined ($cat_form2lemma{$cat}{$form}) && $cat_form2lemma{$cat}{$form} !~ /(^|\|)$qmlemma(\||$)/) {
      $cat_form2lemma{$cat}{$form} .= "|$lemma";
    } else {
      $cat_form2lemma{$cat}{$form} = "$lemma";
      $form_lemma_suffs = "@".$form."###@".$lemma;
      while ($form_lemma_suffs =~ s/^(.)(.+)###\1(.+)/\2###\3/) {
        if (length($2) <= 8) {
          $cat_formsuff_lemmasuff2count{$cat}{$2}{$3}++;
          if ($multiple_lemmas) {
            $cat_formsuff_lemmasuff2count{$cat}{$2}{__ALL__}++;
          }
        }
      }
    }
  }
}

The variable LEFFF is a dictionary composed of 490489 lines.  So, the loop is comparing the words with all the dictionary lines one by one.  This is too much. Any ideas how to optimize this?

Here's a preview of how LEFFF is structured:

touant  VPR touer   0
intimiste   NC  intimiste   1
intimiste   ADJ intimiste   0
phonologiques   ADJ phonologique    1
condescendirent V   condescendre    1
enchaussez  VIMP    enchausser  1
enchaussez  V   enchausser  0
riotiez VS  rioter  1
riotiez V   rioter  0
Eyzin-Pinet NPP Eyzin-Pinet 1
dentaires   NC  dentaire    0
dentaires   ADJ dentaire    1
classe  VIMP    classer 0
classe  VS  classer 1

Well, this is difficult, especially without being a linguist. However, the code is horrid, and I can fix that.

s/ /_/g should be tr/ /_/, as single-character transliterations are more efficient than full-fledged regex matches.

/^(.*?)\t(.*?)\t(.*?)(\t|$)/ || next;
$form = $1; $cats = $2; $lemma = $3;

should be

my ($form, $cats, $lemma) = split /\t/, $_, 4 or next;

The .*? in the regex is a big no-no. It wants to split the line into three parts at tabs – so let's use split.

Certain stuff could be written more readable:

  if ($it_mapping) {
    next if $form =~ /.'$/  # optimized from /^.+'$/
         or $form eq "dato"  && $lemma eq "datare"
         or $form eq "stato" && $lemma eq "stare"
         or $form eq "stata" && $lemma eq "stare"
         or $form eq "parti" && $lemma eq "parto"
         or $cats eq "ART" && $form =~ /^(?:una|la|le|gli|agli|ai|al|alla|alle|col|dagli|dai|dal|dalla|dalle|degli|dei|del|della|delle|dello|nei|nel|nella|nelle|nello|sul|sulla)$/;

    # removing capture groups, using eq where appropriate, using postfix if:
    $cats = "PUNCT"     if $cats =~ /^(?:parentf|parento|poncts|ponctw)$/;
    $cats = "PRON"      if $cats eq 'PRO';
    $cats = "PREDET"    if $cats eq 'ARTPRE';
    $cats = "VERB"      if $cats =~ /^(?:VER|ASP|AUX|CAU)$/;
    $cats = "CONJ"      if $cats eq 'CON';
    $cats = "PREP"      if $cats eq 'PRE';
    $cats = "ADJ"       if $cats eq 'DET';
    $cats = "PRON|CONJ" if $cats eq 'WH';

    # cheaper test (equality) as early as possible
    next if $cats eq "PREP" && $form eq 'ad'
         or $cats eq "ADJ"  && $form =~ /^quest[aei]$/
         or $cats eq "PRON" && $form =~ /^quest[aei]$/
         or $cats eq "ADJ"  && $form =~ /^quell[aei]$/
         or $cats eq "PRON" && $form =~ /^quell[aei]$/
         or $cats eq "CONJ" && $form =~ /^[oe]d$/;
  }

Note that I did not optimize the gargantuan regex – the trie optimization will transform that into a state machine, which is far more efficient. This regex match was also moved before the $cats normalization in order to bail out as early as possible.

$qmlemma = quotemeta ($lemma);

should be

my $qmlemma = quotemeta ($lemma);

because lexical variables can be more efficient, and it makes it easier to reason about the code.

The next part (looping through the $cats) is very hackish, and could be done more efficient with a different data structure.

(I removed my no autovivification recommendation because that behaviour is needed in the else clause again, and it does not effectively save much).

The regex should be changed to /(?:^|[|])$qmlemma(?:[|]|$)/ to avoid unneccessary capturing (and to make it more readable).

The else-branch should be changed to

$cat_form2lemma{$cat}{$form} = $lemma; # avoid unneccessary stringification
my $form_lemma_suffs = '@' . $form . '###@' . $lemma;  # lexical variables!!!
while ($form_lemma_suffs =~ s/^(.)(.+)###\1(.+)/\2###\3/) {
  if (length($2) <= 8) {
    $cat_formsuff_lemmasuff2count{$cat}{$2}{$3}++;
    $cat_formsuff_lemmasuff2count{$cat}{$2}{__ALL__}++ if $multple_lemmas;
  }
}

Most of the changes are meant to improve readability (postfix if, spacing). If I grok that code correctly, then it might be able to change this to

$cat_form2lemma{$cat}{$form} = $lemma; # avoid unneccessary stringification

my ($form_suff, $lemma_suff) = ($form, $lemma);
while (length($form_suff) and substr($form_suff, 0, 1, '') eq substr($lemma_suff, 0, 1, '')) {
  if (length($form_suff) <= 8) {
    $cat_formsuff_lemmasuff2count{$cat}{$form_suff}{$lemma_suff}++;
    $cat_formsuff_lemmasuff2count{$cat}{$form_suff}{__ALL__}++ if $multiple_lemmas;
  }
}

C-style string manipulation with substr is far more efficient than regexes with captures and backrefs.


Brute-force-like solution to Project Euler Problem 5 Project Euler problem #5 is the following:

  2520 is the smallest number that can be divided by each of the numbers from 1 to 10 without any remainder.

  What is the smallest positive number that is evenly divisible by all of the numbers from 1 to 20?

First, a disclaimer: yes, I know that there are much more optimal ways of solving this. This is a very dumb "brute-force" way to solve this that uses very little mathematical intelligence. If I did the analysis correctly (and please correct me if not), this solution is $$O(n^2)$$
With that said, I'm curious if anyone had any feedback on this (and again, I am familiar with the mathematically optimal way to do this). In particular, it seems like there should be a way to eliminate the while loop.

public class SmallestMultiple
{
    public long GetSmallestMultiple(int maxNumber)
    {
        long num = 1;

        for (int i = maxNumber; i >= 1; i--)
        {
            long modulo = num % i;
            if (modulo > 0)
            {
                num *= i;
            }
        }

        long prev = num;

        while (true)
        {
            // If dividing by this number wouldn't cause any "harm" (i.e. it's still actually a multiple of everything),
            // go ahead and do it
            for (int i = 2; i <= maxNumber; i++)
            {
                num /= i;

                for (int j = 2; j <= maxNumber; j++)
                {
                    if (num % j > 0)
                    {
                        num *= i;
                        break;
                    }
                }
            }

            if (prev == num)
            {
                break;
            }
            else
            {
                prev = num;
            }
        }

        return num;
    }
}

And here's my unit test (written in Visual Studio):

[TestClass]
public class SmallestMultipleTests
{
    [TestMethod]
    public void SmallestMultipleTest()
    {
        SmallestMultiple multiple = new SmallestMultiple();

        long result = multiple.GetSmallestMultiple(10);

        Assert.AreEqual(2520, result);

        result = multiple.GetSmallestMultiple(20);

        Assert.AreEqual(232792560, result);
    }
}

The main problem with your code is that it's very hard to understand.

First of all, I'd expect GetSmallestMultiple(10) to return 10. After all, the smallest multiple of 10 is 10, right? That's how your code reads, when it's not really what you want to do. Maybe your signature should look something more like this :

public long GetSmallestCommonMultiple(IEnumerable<int> multiples)

This way, your code reads :

SmallestMultiple.GetSmallestCommonMultiple(Enumerable.Range(1,10));

Now I'd expect 2520 as an answer.

The thing with algorithms is that they can indeed be hard to read, even if it's just a brute force. To counter this, you need strong variable names and a logical flow in your algorithm. (Meaning that just by reading the code I can understand clearly every step of the algorithm)

For example, the code below, if I got it correctly, will find one common multiple (probably not the smallest one) of the multiples. Meaning that, for multiple = 10, we'd have : 10*9*8*7*6

for (int i = maxNumber; i >= 1; i--)
{
    long modulo = num % i;
    if (modulo > 0)
    {
        num *= i;
    }
}

Using the signature I explained at the beginning, we'd get something like that

long commonMultiple = 1;
foreach(var multiple in multiples)
    if(commonMultiple % multiple > 0)
        commonMultiple *= multiple;

Is it much clearer? Nope. What could we do to make it better? Nothing much, right? In such situations, add comments is a good thing to do.

Basically, I think you need to think of better variable names and try to make the algorithm more apparent in your code, now it's kind of hidden behind your code. I know this sounds weird, but usually you should be able to read code once or twice and get what the algorithm does. Now, I can't. I know what you're doing because I know what method you employed, but if I was to read this without barely no mathematic background I'd struggle to understand what's happening.


Table class to fill array structure with values The code creates a table and adds rows and columns.

Does it make sense to write the code like this?
Would you design it differently?
Is there a better OOP approach to this?

class Table
  attr_reader :grid

  def initialize(rows: , columns:)
    @grid = construct(rows, columns)
  end

  def construct(rows, columns)
    raise 'arguments must be arrays' unless rows.is_a?(Array) && columns.is_a?(Array)
    table = []
    rows = [] if rows.nil?
    rows.each do |row|
      table << columns.map do |c| c end
    end
    table
  end
end

Here are the specs which describe what it is doing:

require 'spec_helper'
require_relative '..\test'

RSpec.describe Table do

  it 'is empty' do
    table = Table.new(rows: [], columns: [])
    expect(table.grid).to eq []
  end

  it 'raises if no array is given' do
    expect {
      Table.new(rows: [], columns: 1) }.to raise_error RuntimeError
    expect {
      Table.new(rows: 1, columns:[]) }.to raise_error RuntimeError
  end

  it 'has several rows and columns' do
    row = double('row')
    column = double('column')
    col = Column.new.value = 14
    table = Table.new(rows: [1, 3, 3], columns: [1, col])
    expect(table.grid).to eq [
      [1, 2],
      [1, 2],
      [1, 2]
    ]
  end
end

Refactored Code

This is a possible way to refactor the code to meet the style guidelines of Ruby using Rubocop.

# frozen_string_literal: true

# A simple table with rows and cols
class Table
  attr_reader :grid

  def initialize(rows:, columns:)
    @grid = construct(rows, columns)
  end

  def construct(rows, columns)
    unless rows.is_a?(Array) && columns.is_a?(Array)
      raise 'arguments must be arrays'
    end

    table = []
    rows = [] if rows.nil?
    rows.each do |_row|
      table << columns.map { |c| c }
    end
    table
  end
end

Style Considerations

A guard clause should be appended wih an empty line. Also, since the line is over 80 characters, split it up in multi-line.

raise 'arguments must be arrays' unless rows.is_a?(Array) && columns.is_a?(Array)
table = []

unless rows.is_a?(Array) && columns.is_a?(Array)
  raise 'arguments must be arrays'
end

table = []

You have an unused block argument row and the map should be rewritten using {..}.

rows.each do |row|
  table << columns.map do |c| c end
end

rows.each do |_row|
  table << columns.map { |c| c }
end

General Guidelines and Conventions

The tool also complained about the following guidelines.

use 2 instead of 4 white spaces for indentation
remove any trailing white space
remove space before comma
add a frozen string literal comment justification
add top level class documentation comment


Python beginner's body fat calculator I'm very very fresh to programming. This is one of my first experiments with Python and I'm wondering in what ways I could have made this program less clunky. Specifically, is there a way that I could have used classes instead of defining my x, y, and z variables globally?

def getx():
    try:
        global x
        x = float(raw_input("Please give your weight (pounds): "))
        return x
    except ValueError:
        print("Use a number, silly!")
        getx()
def gety():
    try:
        global y
        y = float(raw_input("what is your current body fat percentage? (>1): "))
        return y
    except ValueError:
        print("Use a number, silly!")
        gety()
def getz():
    try:
        global z
        z = float(raw_input("What is your desired body fat percentage? (>1): "))
        return z
    except ValueError:
        print("Use a number, silly!")
        getz()
def output():
    getx()
    gety()
    getz()
    A = (x*(y/100-z/100))/(1-z/100)
    B = x - A
    print("Your necessary weight loss is %.1f pounds, and \
your final weight will be %.1f pounds" % (A,B))
    more()
def more():
    again = raw_input("Calculate again? ")
    if again.lower() == "yes" or \
       again.lower() == "y" or \
       again.lower() == "sure" or \
       again.lower() == "ok" or \
       again.lower() == "" or \
       again.lower() == "okay":
        output()
    elif again.lower() == "no" or \
         again.lower() == "n" or \
         again.lower() == "nah" or \
         again.lower() == "nope":
            end()
    else:
        more()
def end():
    print("Ok, see ya later!")
output()

all of your functions seem to do the same thing with a different message, so why not condense them and take the message as a parameter?

def get_num(msg):
    num = None
    while num is None:
        try:
            num = float(raw_input(msg))
        except ValueError:
            print("Use a number, silly!")

    return num

and then

def output():
    x = get_num('Please give your weight (pounds): ')
    y = get_num('what is your current body fat percentage? (>1): ')
    z = get_num('What is your desired body fat percentage? (>1): ')
    A = (x*(y/100-z/100))/(1-z/100)
    B = x - A
    print("Your necessary weight loss is %.1f pounds, and \
your final weight will be %.1f pounds" % (A,B))
    more()

in your more function you can condense your ifs with the in operator

def more():
    again = raw_input("Calculate again? ")

    if again.lower() in ["yes", "y", "sure" , "ok", "", "okay"]:
        output()
    elif again.lower() in ["no", "n", "nah", "nope"]:
        end()
    else:
        more()


Exception Handler on a Switch I am working on rewriting an application in Laravel 5.1. I am new to the exception handling technique introduced in 5.0. I have overlooked taking advantage of throwing/catching exceptions frequently in the past, but am working on this now. I am most likely not up to speed on some of the best practices.

public function render($request, Exception $e)
{

    switch (true) {

        case $e instanceof Exceptions\Auctions\AuctionTypeException:
            // Should I throw a new exception here? Is passing the old exception message to a new exception good practice?
            throw new Exceptions\Notifications\AlertException($e->getMessage());
            break;

        case $e instanceof Exceptions\Auctions\AuctionArgumentException:
            // Should I throw a new exception here? Is passing the old exception message to a new exception good practice?
            throw new Exceptions\Notifications\AlertException($e->getMessage());
            break;

        ////////////////////////////////////////////
        // Custom errors to display in error view //
        ////////////////////////////////////////////
        case $e instanceof Exceptions\Notifications\AlertException:

            return response()->view('errors.notification', ['message' => $e->getMessage(), 500]);
            break;

        //////////////////////////////////////////////
        // Unknown exceptions are rendered normally //
        //////////////////////////////////////////////
        default:
            return parent::render($request, $e);
    }

}

Docs for the exception handler are here.

While most of my exceptions will fall under the "explicitly ignore and continue" umbrella, I feel like I will need to handle my exceptions in a more abstract/polymophic way later down the road, so I am preparing now. This is a huge app, and I will have several custom Exceptions.

This code currently works and behaves as excepted. Exceptions that I explicitly state I want to do something special with can be added as a case, and if it is new, or I just want to handle it normally...it falls under the default.

I would like to know if I am going down the right path - to me, this seems like a fairly clever way to handle this. Is it a good idea to catch an exception, only to throw another exception with the previous exceptions message? Are there any pitfalls to checking the instance and evaluating true like this?

There is nothing wrong with rethrowing an exception of a different type, the problem lies with how you are doing it. According to the PHP docs, you can pass a previous exception as a second argument to the new exception. This preserves the original stack trace. I would also provide a new error message as well:

throw new MyException("A new error summary", $e);

A new take on your switch statement:

public function render($request, Exception $e)
{

    switch (true) {

        case $e instanceof Exceptions\Auctions\AuctionTypeException:
            throw new Exceptions\Notifications\AlertException("An auction type failed to save", $e);
            break;

        case $e instanceof Exceptions\Auctions\AuctionArgumentException:
            throw new Exceptions\Notifications\AlertException("An auction argument was not valid", $e);
            break;

        ////////////////////////////////////////////
        // Custom errors to display in error view //
        ////////////////////////////////////////////
        case $e instanceof Exceptions\Notifications\AlertException:

            return response()->view('errors.notification', ['message' => $e->getMessage(), 500]);
            break;

        //////////////////////////////////////////////
        // Unknown exceptions are rendered normally //
        //////////////////////////////////////////////
        default:
            return parent::render($request, $e);
    }

}


Interfaces and Classes between Projects My application's structure looks like this:

Solution: MySolution

Projects in MySolution: Host, Business, Server.

References between the projects looks like this:

Host <-- Business --> Server

(Business references Host and Server but Host does not reference Server and vice versa because this causes a circular-)

So, I have some Database stuff in Server. I have a property, DBConnectionString - which should contain the connection string. What I want to do is to set this property from the Host but I want to keep all methods that have anything to do with the database in the Server project.

Keeping interfaces in mind - so that I can also at a later stage just create an web application.

I have some code and it's working but I don't know if this is the right way.

I have this interface in Library (connection between the other two projects):

    public interface IDBUtils
    {
        string DBConnectionString { get; set; }

        bool OpenConnection();

        bool CloseConnection();

        bool ExecuteSQL(string sql);

        bool CreateDatabase(string servername, string databaseName, string databaseDataPath);

        bool CreateDatabaseUser(string servername, string username, string password);
    }

Then the class in Library that has the code for the interface:

public string DBConnectionString
{
    get
    {
        return DBService.DB.DBUtils.DBConnectionString;
    }
    set
    {
        DBService.DB.DBUtils.DBConnectionString = value;
    }
}

public bool OpenConnection()
{
    return DBService.DB.DBUtils.OpenConnection();
}

public bool CloseConnection()
{
    return DBService.DB.DBUtils.CloseConnection();
}

public bool ExecuteSQL(string sql)
{
    return DBService.DB.DBUtils.ExecuteSQL(sql);
}

public bool CreateDatabase(string servername, string databaseName, string databaseDataPath)
{
    return DBService.DB.DBUtils.CreateDatabase(servername, databaseName, databaseDataPath);
}

public bool CreateDatabaseUser(string servername, string username, string password)
{
    return DBService.DB.DBUtils.CreateDatabaseUser(servername, username, password);
}

I'm only going to show the property and one method on the server otherwise this is going to get too long.

On the Server:
Property:

public static string DBConnectionString { get; set; }

The OpenCOnnection() Method:

        public static bool OpenConnection()
        {
            bool success = false;
            try
            {
                if (DBConnectionString.Length > 0)
                {
                    conn = new SqlConnection(DBConnectionString);
                    conn.Open();
                    success = true;
                }
            }
            catch { /* Do nothing... */ }
            return success;
        }

To set the property from Host on Server:

It uses the interface on Library:

iDBUtils.DBConnectionString = connectionString.ToString();

To execute a method from Host on Server:

iDBUtils.OpenConnection();

So as you can see, Library is the "Middle Man".

Naming

IDBUtils is a name for a namespace, but for an interface representing a database system it is poorly named. Better use something like IDatabase, IDatabaseServer or IDatabaseSystem.

OpenConnection()

This method can be simplified by removing the success variable and also using a guard clause, which reduces horizontal spacing.

public static bool OpenConnection()
{
    if (String.IsNullOrWhiteSpace(DBConnectionString.Length))
    {
        return false;
    }

    try
    {
        conn = new SqlConnection(DBConnectionString);
        conn.Open();
        return true;
    }
    catch { /* Do nothing... */ }
    return false;
}


Reverse Polish Notation evaluator in Haskell I've created an RPN evaluator in Haskell, as an exercise because I'm new to this language.

It runs well:

$ ./rpn
2 3 4 5 + - +
-4

And the source code:

{-# LANGUAGE BangPatterns #-}
import Data.String
import System.IO

data Token = TNum Int | TOp Operator
data Operator = Add | Sub | Mul | Div

main :: IO ()
main = do
    line <- getLine
    let tokens      = tokenise line
        (numc, opc) = countTok tokens
        !junk       =
            if numc == opc + 1
                then ()
                else error "Not a correct expression."
    print $ eval [] tokens

tokenise :: String -> [Token]
tokenise = map str2tok . words

eval :: [Int] -> [Token] -> Int
eval (s:_) []                   = s
eval stack (TNum t:ts)          = eval (t : stack) ts
eval (x:y:stacknoxy) (TOp t:ts) = eval (applyOp t y x : stacknoxy) ts

str2tok :: String -> Token
str2tok tkn@(c:_)
    | c `elem` ['0'..'9'] = TNum (read tkn :: Int)
    | otherwise = TOp $ case tkn of
        "+" -> Add
        "-" -> Sub
        "*" -> Mul
        "/" -> Div
        _   -> error $ "No such operator " ++ tkn

applyOp :: Operator -> Int -> Int -> Int
applyOp Add a b = a + b
applyOp Sub a b = a - b
applyOp Mul a b = a * b
applyOp Div a b = a `div` b

countTok :: [Token] -> (Int, Int)
countTok [] = (0, 0)
countTok (t:ts) =
    let (x, y) = case t of
            TNum _ -> (1, 0)
            _      -> (0, 1)
     in (x, y) `addPair` countTok ts

addPair :: (Num a, Num b) => (a, b) -> (a, b) -> (a, b)
addPair (x, y) (z, w) = (x + z, y + w)

How can this code be improved? I hope my implementation is elegant, and if it isn't - what are the ways to clean it up?  In particular, I really don't like the errors because they're just ugly:

./rpn
5 6 + +
rpn: Not a correct expression.
CallStack (from HasCallStack):
  error, called at rpn.hs:16:22 in main:Main

I know they can be replaced with fail, which has a much nicer output, but from what I've read it can only be done inside a function that returns IO ().

First, I would convert all of the functions which might throw an error to return some kind of failable type, like Maybe Int or Either String Token. This includes str2tok, tokenize, and eval. This would also remove the need for the countTok function, since the program can just return an error value from eval instead. With the help of the Monad instances for these error types, this is a relatively simple change.

Next, I would use isDigit from Data.Char instead of c `elem` ['0'..'9'] because isDigit makes less comparisons in order to determine if it's a digit.

Lastly, I would change the Operator type to the function type Int -> Int -> Int. This will remove the need for applyOp and will consolidate all the places that would be necessary to change if you wanted to extend the program to accept more operators.

import Data.Char (isDigit)

data Token = TNum Int | TOp (Int -> Int -> Int)

main :: IO ()
main = do
    line <- getLine
    either putStrLn print $ do
        tokens <- tokenize line
        eval [] tokens

tokenize :: String -> Either String [Token]
tokenize = mapM str2tok . words

str2tok :: String -> Either String Token
str2tok tkn
    | (c:_) <- tkn, isDigit c = Right $ TNum (read tkn)
    | otherwise = TOp <$> case tkn of
        "+" -> Right (+)
        "-" -> Right (-)
        "*" -> Right (*)
        "/" -> Right div
        _   -> Left $ "No such operator " ++ tkn

eval :: [Int] -> [Token] -> Either String Int
eval (s:_) []                   = Right s
eval stack (TNum t:ts)          = eval (t : stack) ts
eval (x:y:stacknoxy) (TOp t:ts) = eval (t y x : stacknoxy) ts
eval _ _                        = Left "Not a correct expression."