Checking whether a table has modified rows The company I am with has a lot of old code migrated over from ALPHA Basic, written back in the 1980s. As development languages have evolved (should I say, "died off"?) the code has been migrated to newer languages.

The company also hires a lot of new graduates from a local college. Sometimes, the code is written in a textbook fashion. I see a lot of code that could be written differently.

Here is one piece of code that I have encountered in a project I maintain:

Public ReadOnly Property HasModifiedRows() As Boolean
    Get
        Dim dt As DataTable

        dt = Nothing
        Try
            dt = Me.GetTableObj.GetChanges(DataRowState.Modified)
            If dt IsNot Nothing Then
                Return True
            Else
                Return False
            End If
        Finally
            If dt IsNot Nothing Then
                dt.Dispose()
                dt = Nothing
            End If
        End Try
    End Get
End Property

The code looks deliberately spelled out, step for step. This could be to help our new developers understand what is going on, or it could be code that was migrated over from older languages that required all variables be defined at the top.

I do not know why, but seeing all of the unnecessary lines of code is like fingernails on a chalkboard.

My planned rewrite

I am constantly compelled to rewrite pieces of code like that above to this:

Public ReadOnly Property HasModifiedRows() As Boolean
    Get
        Using table As DataTable = GetTableObj.GetChanges(DataRowState.Modified)
            Return True
        End Using
        Return False
    End Get
End Property

Question and concerns

Is one design pattern better than another?
Is there a performance difference in the two?
Would my version with the using syntax have any unanticipated results?

As I understand the VB Using Statement, if an object enters a using block, it cannot be null and it will always dispose of itself whenever it goes out of scope. I could of course be wrong though. I do not know how to guarantee this in .Net.

Should I leave the old code alone?

If there is any benefit to rewriting it, I am certainly happier with the streamlined look.

I'd really like to get some opinions on if I am doing any good by changing the code. If I am doing harm, I certainly would like to know!

Is one design pattern better than another?

Every code statement is a potential defect. If you can express yourself in a shorter form without sacrificing readability, then by all means you should. The less code, the less to read and debug, the fewer suspicious elements.

  Is there a performance difference in the two?

I don't actually know VB, but this is a common pattern in many languages, and it's clear that the performance should be the same, or the second might be even faster, thanks to some optimization magic by the compiler.

  Would my version with the using syntax have any unanticipated results?

Increased understanding among readers? You changed not just the writing style, but you also gave the variables more meaningful names. It's intuitive and a lot easier to read than the first one. Both seem to accomplish exactly the same thing.

  Should I leave the old code alone?

Probably. Don't fix what ain't broken. Although the original method is not implemented pretty, from the outside it doesn't really matter: it has a good name, and it does exactly what the name says and nothing else, it works. It's nice to make it nicer, but there isn't really a need. Now that you looked at it closely and know that it works, try to treat it as a black box, and remember not to open again. Use your energy for more creative tasks.


Reliably setting the system clipboard in a spawned thread and having the spawner wait Checklist from FAQ for proper posting.

Does my question contain code?
   - YES
Did I write that code?
   - YES
Is it actual code from a project rather than pseudo-code or example code?
   - Strait from actual code, minus a few functions that do not apply to the Q
To the best of my knowledge, does the code work?
   - To the best of my knowledge it works most of the time, other times it
     fails to do what I ask of it but produces no runtime errors.
Do I want feedback about any or all facets of the code?
   - Just about how to reliably wait for the clipboard to be set and
     reliably set it while I am waiting.

I am trying to reliably set the system clipboard text in my program but keep coming back with confusing results. Those results being it sometimes setting the text and sometimes not setting it. Here is where I create a thread of type ClipSetThread and tell it to set the system clipboard and also set a global variable called waitForMe in the main Application.

public void setClip(String arg) // Starts a clip setting thread
{
    CLUtilCompact.waitForMe = true;
    ClipSetThread set = new ClipSetThread(theApp);
    set.arg = arg;
    set.start();
    while(CLUtilCompact.waitForMe){}
}

And here is the class ClipSetThread

import java.awt.Toolkit;
import java.awt.datatransfer.Clipboard;
import java.awt.datatransfer.DataFlavor;
import java.awt.datatransfer.StringSelection;
import java.awt.datatransfer.Transferable;

public class ClipSetThread extends Thread
{
String arg = "";
Clipboard sysClip = null;
CLUtilCompact theApp = null;
public ClipSetThread(CLUtilCompact app)
{
    theApp = app;
    theApp.updateOutput("Set Clip ID:" + this.getId() + "|CREATED");
    sysClip = Toolkit.getDefaultToolkit().getSystemClipboard();
}
public void run()
{
    setClip2(arg);
    CLUtilCompact.waitForMe = false;
    return;
}
public void setClip2(String arg)
{
    while(true)
    {
        try
        {
            sysClip.setContents(new StringSelection(arg), null);
        }
        catch(Exception e)
        {
            try {Thread.sleep(20);} catch (InterruptedException e1) {}
            continue;
        }
        break;
    }
    return;
}
}

It seems that even with the while loop to wait for the waitForMe flag the setClip function will return and allow the application to try and continue what it is doing even though the clipboard is not set.

After the clipboard is set I almost always try to paste what is there; I usually call Thread.sleep(1000) between a call to setClip and my call to paste(). (Paste simply simulates ctrl+V with the java Robot class)

This code is quite confusing. Do you know about parallel threads mechanisms?

CLUtilCompact.waitForMe = true;
set.start();

A new thread is started now and executes the run method from the interface. It is not defined when the thread is run, how fast and when he will execute any statements (and can not, the OS is responsible for this, not the JVM)

So, sometimes, this lines is called first:

while(CLUtilCompact.waitForMe){}

Sometimes this line is called first:

CLUtilCompact.waitForMe = false;

Even if the second line is called first, nobody knows when the field update will propagate to the other thread.

What we can do about this? In general, nothing. This is the (desired) behavior of threads.
For your problem, you can either block the application until the job is done, or hope that it is done fast enough asynchronously. You could implement some logic, which will display some error message after a given time. But this is quite a bad solution for slow system or systems under heavy load.

If you want to do something after the run method has finished, try SwingWorker (read the java documentation).

Some more things:

ClipSetThread set = new ClipSetThread(theApp);
set.arg = arg;

Use the constructor, add a second argument.

while(true)
{
    try
    {
        sysClip.setContents(new StringSelection(arg), null);
    }
    catch(Exception e)
    {
        try {Thread.sleep(20);} catch (InterruptedException e1) {}
        continue;
    }
    break;
}
return;

Without discussing the pattern, this could be more readable:

boolean isSuccessful = false;
while(!isSuccessful) {
    try {
        sysClip.setContents(new StringSelection(arg), null);
        isSuccessful = true;
    }
    catch(Exception e) {
        try {Thread.sleep(20);} catch (InterruptedException e1) {} //you could use a method wait(seconds) for this
    }
}
return;

And you should, if possible, document inside the catch block why you are doing nothing there.


Bundling JavaScript resources for an MVC app This is my first MVC app. I'm looking for best practice feedback on how to best bundle the files that I need, the order to bundle them, the placement of the @render methods within the containing html page.

BundleConfig

namespace StateAssessment
{
    using System.Web.Optimization;

    public class BundleConfig
    {
        // For more information on bundling, visit http://go.microsoft.com/fwlink/?LinkId=301862
        public static void RegisterBundles(BundleCollection bundles)
        {
            bundles.Add(new ScriptBundle("~/bundles/jquery").Include(
                        "~/Scripts/jquery-{version}.js"));

            bundles.Add(new ScriptBundle("~/bundles/jqueryval").Include(
                        "~/Scripts/jquery.validate*"));

            bundles.Add(new ScriptBundle("~/bundles/jquery-ui").Include(
                        "~/Scripts/jquery-ui-{version}.js"));

            // Use the development version of Modernizr to develop with and learn from. Then, when you're
            // ready for production, use the build tool at http://modernizr.com to pick only the tests you need.
            bundles.Add(new ScriptBundle("~/bundles/modernizr").Include(
                        "~/Scripts/modernizr-*"));

            bundles.Add(new ScriptBundle("~/bundles/bootstrap").Include(
                    "~/Scripts/bootstrap.js",
                    "~/Scripts/respond.js"));

            bundles.Add(new StyleBundle("~/Content/css").Include(
                    "~/Content/bootstrap.css",
                    "~/Content/site.css"));

            // Set EnableOptimizations to false for debugging. For more information,
            // visit http://go.microsoft.com/fwlink/?LinkId=301862
            #if DEBUG
                BundleTable.EnableOptimizations = false;
            #else
                BundleTable.EnableOptimizations = true;
            #endif
        }
    }
}

_layout.cshtml

<!DOCTYPE html>
<html>
<head>
    <meta charset="utf-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <title>State Assessment Recovery System - @ViewBag.Title</title>
    <link href="~/Content/themes/base/all.css" rel="stylesheet" />
    @Styles.Render("~/Content/css")
    @RenderSection("Styles", required: false)

    @using StateAssessment.CommonHelpers.HelperControls;

    <meta name="description" content="The description of my page" />
</head>
<body>
    @RenderBody()

    @Scripts.Render("~/bundles/modernizr")
    @Scripts.Render("~/bundles/jquery")
    @Scripts.Render("~/bundles/jquery-ui")
    @Scripts.Render("~/bundles/jqueryval")
    @Scripts.Render("~/bundles/bootstrap")
    @RenderSection("scripts", required: false)
</body>
</html>

Index.cshtml

@model StateAssessment.ViewModels.StateAssessment.Index

@section Styles {
    <link href="@Url.Content("~/Content/ViewSpecific/StateAssessment/IndexItem.css")"  rel="stylesheet"/>
}

    <!--markup goes here -->

    @section Scripts {

         <script src="~/Scripts/ViewSpecific/StateAssessment/Index.js" type="text/javascript">
         }
        </script>
    }

The best practice moved from using dotNet code for bundling, to use tools for this.

The most popular and best practised tools are:

gulp
grunt

Both are tools that work on top of nodejs, an easy to install app.

Use either of these tools to minify and bundle your js and css and reference to result file.

You can make the tooling run on build events of your project to always have it bundled

But, best practise here is to use development files in development (to  keep yourdev cycle a short as possible). And in production use the bundled and minified version.


StopWatch app in android Activity

public class StopWatchActivity extends AppCompatActivity {

    int seconds;
    boolean running;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_stop_watch);
        runTimer();
    }

    public void startTimer(View view){
        running = true;
    }

    public void stopTimer(View view){
            running = false;
        }

    public void resetTimer(View view){
            running = true;
            seconds = 0;
        }

    public void runTimer(){
        final TextView textView = (TextView) findViewById(R.id.timer);
        final Handler handler = new Handler();
        handler.post(new Runnable() {
            @Override
            public void run() {
                    int hours = seconds / 3600;
                    int minutes = (seconds % 3600) / 60;
                    int sec = seconds % 60;
                    String time = String.format("%d:%02d:%02d", hours, minutes, sec);
                    textView.setText(time);
                if(running) {
                    seconds++;
                }
                handler.postDelayed(this, 1000);
            }
        });

    }
}

I am learning android. I made this app work from my reference book.

I want to know few things.

How do I improve my code?
Why should I use handler?
If other wasy to do this, thenWhy does this approach fails to update the layout?
What else should I consider in this app in terms of performance?
If third question cannot be answered in this domain, please leave that.

I have to say your code is clean and well thought-through. There is always space of improvement though.

Cut the fluff. Reduce your access modifiers. There is no (visible) reason for seconds nor running not to be private.

And no reason why timer-controlling methods have to be public, either. Indeed, this is not how you would want the external word to communicate with your Activity - in the Android world, Activities have their own life-cycle and they get shut down and recreated by the OS, meaning you don't normally want to hold on to any particular instance of an Activity.

(Also, don't use redundant arguments: your timer-controlling methods don't make any use out of your View parameter.)

Due to the lifecycle thing I mentioned - more info at  http://developer.android.com/training/basics/activity-lifecycle/index.html - you should check whether the Activity is still there before you call textView.setText(time)!

Attempting to update a UI that's no longer in sight (and therefore gone) will result in a crash. How could it happen? Very simple - let's say your timer is running, and there's an incoming call in between two ticks. Or user switches to another app, or dims the screen. Etc. And it's fairly nondeterministic, especially when the device is running short on memory.

Using a Handler is a convenient way of sending actions to the thread message queue. And this trick with your Handler calling itself is an easy way of ensuring that "ticks" repeat over and over (without using the actual Timer, whose API is a bit clunky).

However, this Handler is a perpetuum mobile. It will prevent the Activity from ever getting garbage-collected. Even if it refrains from touching the UI if it no longer exists, it would keep on going. You should kill it if user exits the Activity - see https://stackoverflow.com/a/20377022/168719

To minimize these problems, I'd recommend extracting the timer logic into a separate class, and have your StopWatchActivities (note the plural: you shouldn't assume there'll only be a single instance!) subscribe and unsubscribe from it in their onResume / onStop methods.

Another thing: a Handler by default runs on the same thread on which it was created, in this case it's the UI thread. This is okay in this case because the task is light-weight and pretty much limited to updating the UI (which has to be done on the UI thread anyway). If it was computationally expensive though, you'd have to delegate it to another thread, but then make sure to eg. call the textView.setText(time) via  Activity.runOnUiThread().

I would also have a few cosmetic remarks related to naming.

stopTimer should rather be pauseTimer, because it doesn't reset it.
having variables named sec and seconds is a bit confusing. I'd prefer seconds and totalSeconds or secondsElapsed - it distinguishes them better.

I'm not sure what's exactly wrong with the code from your link - for starters, I can't see where the 1 second delay between ticks is enforced?? - but it proves my point as for why having sec next to seconds isn't a good idea:

int sec = seconds % 60;
String time = String.format("%02d:%02d:%02d", hours, minutes, seconds);

Surely the author meant to pass sec, not seconds, to String.format, no? :)


Counting contiguous subarrays with a negative sum I'm doing a coding challenge that asks to count the number of contiguous subarrays that have a negative sum:

A subarray of an n-element array is an array composed from a contiguous block of the original array'selements. For example, if array = [1,2,3], then the subarrays are [1], [2], [3], [1,2], [2,3], and [1,2,3]. Something like [1,3] would not be a subarray as it's not a contiguous subsection of the original array.
The sum of an array is the total sum of its elements.
An array's sum is negative if the total sum of its elements is negative.
An array's sum is positive if the total sum of its elements is positive.
Given an array of integers, find and print its number of negative subarrays on a new line.

I wrote this method and I'm trying to figure out the runtime for it but I'm not sure how to handle the loop and the recursion. Assume the length of array a is > 0.
int numNegatives(int[] a){

    if(a.length == 1){
        if(a[0] < 0){ return 1; } else { return 0; }
    }

    int count = 0;
    int sum = 0;

    for(int i = 0; i < a.length; i++){ //N
        sum += a[i];
        if(sum < 0){ count++; }
    }
    count += numNegatives(Arrays.copyOfRange(a, 1, a.length));//N-1

    return count;
}

The for loop will be O(N), since it loops over the array, and I believe the recursion call will then be O(N-1). Since the recursion call is outside the for loop, does that make the total O(N)? Most solutions to this challenge are two nested for loops with O(N²) runtime, and so I'm not sure if I actually made any improvement. Is it worse than O(N²)?

To get a rough idea, you could consider: how 'deep' does the recursion go, and for each layer, how much work has to be done? See for example the tree in 8.2 Worst-case partitioning, as this is similar to what we have here.

Recurse for every element in the list, so the depth of the recursion tree is of the order n.

For each layer, there is the for loop, which is of the order n, and the array copying, which is also of the order n. So for each layer, there of the order O(n) amount of work to do.

So the algorithm does have complexity O(n^2). Also, is it really necessary to make the copy of the array each time? If your function had a second int start parameter (which is incremented with each recursive call), you could begin the for loop from further down the array with each recursive call, without the need for copies.

This might be a bit crude (and perhaps misleading if you choose the wrong scale), but you can always test it out with some different input sizes to see what happens. Note that with arrays above around 25000 elements I was getting StackOverflowErrors (although rereading the HR task, it was given the array size <= 100). You could do this by generating some random input of different sizes, and timing your function:


Guessing the word game I'm done with this "Guessing word game" but I have a very ugly method named startGuessing().

I tried to make it cleaner and more readable but I couldn't. How I can improve this method?

public class GameHangman {

private static String[] WORDS = {"programming", "fish", "cairo", "arabs", "quraan", "sunnah",
        "editor", "photography", "artist", "software", "write", "read", "gaming",
        "lunch", "participate", "lunch", "dinner", "house", "books", "animals"};

public static void startGame() {

    Scanner input = new Scanner(System.in);

    //Choose a random word
    String word = chooseWord();

    //Start guessing game with chosen word and count missed times
    int missed = startGuessing(word, input);

    //Display result
    System.out.println("The word is " + word + (missed == 0 ? " you are correct" : " you missed " + (missed == 1 ? " one time" : " " + missed + " times")));

    //Ask for another game
    askForAnotherGame(input);
}

private static void askForAnotherGame(Scanner input) {
    System.out.print("Do you want to start another word? Enter y or no>");
    char gameCase = input.nextLine().charAt(0);
    if (gameCase == 'y')
        startGame();
    else
        System.exit(1);
}

private static int startGuessing(String originalWord, Scanner input) {
    StringBuilder replacedWord = new StringBuilder(hideRealWord(originalWord));
    int missing = 0;
    boolean wrongGuessing;
    int wordLength = originalWord.length();

    while (wordLength > 0) {
        System.out.print("(Guess) Enter a letter in word " + replacedWord.toString() + " ");
        char ch = input.nextLine().charAt(0);
        wrongGuessing = true;
        for (int j = 0; j < originalWord.length(); j++) {
            if (originalWord.charAt(j) == ch) {
                replacedWord.setCharAt(j, ch);
                wrongGuessing = false;
                wordLength--;
            }
            if (ch == replacedWord.charAt(j))
                System.out.println(ch  + " Already in the word");
        }
        if (wrongGuessing) {
            System.out.println(ch + " is not in the word");
            missing++;
        }

    }
    return missing;
}

private static String hideRealWord(String originalWord) {
    //System.out.println(originalWord);
    for (int i = 0; i < originalWord.length(); i++) {
        char ASTERISK = '*';
        originalWord = originalWord.replace(originalWord.charAt(i), ASTERISK);
    }
    return originalWord;
}

private static String chooseWord() {
    return WORDS[(int) (Math.random() * WORDS.length)];
    }
}

Break the method down

You can improve the method by trying to put it in smaller methods.

There is a while loop that is a good candidate for this.

The code becomes:

private static int startGuessing(String originalWord, Scanner input) {
        StringBuilder replacedWord = new StringBuilder(hideRealWord(originalWord));
        int missing = 0;
        boolean wrongGuessing;
        int wordLength = originalWord.length();

        while (wordLength > 0) {
            guess();
        }
        return missing;
    }

Use fields in the GameHangman object

Now, you will notice things will break down, as you need the state from your method in the guess() method. So, you'll need to pass all the information around. You will notice this is kind of hard.

Here comes OOP around. You can store the state in an Object. Currently, your game is all static methods. If you start re-writing the game to use an instance of HangmanGame and use fields it will become easier.

For example, good candidates for object-state are:

Scanner
Current word
Original word
Number of incorrect guesses
Remaining length of the word

Now, it becomes something like this:

public class GameHangman {

    private final static String[] WORDS = { "programming", "fish", "cairo", "arabs", "quraan", "sunnah", "editor",
            "photography", "artist", "software", "write", "read", "gaming", "lunch", "participate", "lunch", "dinner",
            "house", "books", "animals" };

    private Scanner input;
    private String originalWord;
    private int missing = 0;
    private boolean wrongGuessing;
    private int wordLength ;

    public GameHangman()
    {
        this.input = new Scanner(System.in);
    }

    public void start() {

        boolean keepPlaying = true;

        while (keepPlaying)
        {
            // Choose a random word
            this.originalWord = chooseWord();

            // Start guessing game with chosen word and count missed times
            int missed = startGuessing();

            // Display result
            System.out.println("The word is " + originalWord + (missed == 0 ? " you are correct"
                    : " you missed " + (missed == 1 ? " one time" : " " + missed + " times")));

            // Ask for another game
            keepPlaying = userWantsAnotherGame();

        }
    }

    private boolean userWantsAnotherGame() {
        System.out.print("Do you want to start another word? Enter y or no>");
        char gameCase = this.input.nextLine().charAt(0);
        return (gameCase == 'y');
    }

    private  int startGuessing() {
        StringBuilder replacedWord = new StringBuilder(hideRealWord(this.originalWord));
        this.missing = 0;
        this.wordLength = this.originalWord.length();

        while (wordLength > 0) {
            guess(replacedWord);

        }
        return missing;
    }

    private void guess(StringBuilder replacedWord) {

        System.out.print("(Guess) Enter a letter in word " + replacedWord.toString() + " ");
        char ch = this.input.nextLine().charAt(0);
        wrongGuessing = true;
        for (int j = 0; j < this.originalWord.length(); j++) {
            if (this.originalWord.charAt(j) == ch) {
                replacedWord.setCharAt(j, ch);
                wrongGuessing = false;
                wordLength--;
            }
            if (ch == replacedWord.charAt(j))
                System.out.println(ch + " Already in the word");
        }
        if (wrongGuessing) {
            System.out.println(ch + " is not in the word");
            missing++;
        }
    }

    private static String hideRealWord(String originalWord) {
        String hiddenWord = originalWord;
        for (int i = 0; i < originalWord.length(); i++) {
            char ASTERISK = '*';
            hiddenWord = hiddenWord.replace(hiddenWord.charAt(i), ASTERISK);
        }
        return hiddenWord;
    }

    private static String chooseWord() {
        return WORDS[(int) (Math.random() * WORDS.length)];
    }
}

More improvements

Still lots of room for improvement, but at least the startGuessing is shorter and more readable.

More things to improve:

Think of which variables you really need. For example, you don't really need word-length; you just need to check if the currently-guessed word is equal to the original word.
String is immutable, so you use a StringBuilder. While this works, it feels kind of wrong to replace characters in a StringBuilder. Let's use a char[] to store the current state of the word that is being guessed. For easier comparison and character comparison, we can put the original word in a char[] as well.
The main loop of starting a game is called recursively. This might cause a stack overflow if you play many games. Let's convert it to a while loop.

Now, keeping close to your solution, but with these improvements:

import java.util.Scanner;

public class GameHangman {

    private final static String[] WORDS = { "programming", "fish", "cairo", "arabs", "quraan", "sunnah", "editor",
            "photography", "artist", "software", "write", "read", "gaming", "lunch", "participate", "lunch", "dinner",
            "house", "books", "animals" };

    private Scanner input;
    private char[] originalWord;
    private char[] currentWord;
    private int misses;

    public GameHangman() {
        this.input = new Scanner(System.in);
    }

    public void start() {

        boolean keepPlaying = true;

        while (keepPlaying) {
            // Choose a random word
            this.originalWord = chooseWord();
            this.currentWord = hideRealWord(originalWord);
            this.misses = 0;
            while (!this.originalWord.equals(this.currentWord)) {
                guess();
            }

            // Display result
            System.out.println("The word is " + String.valueOf(this.originalWord) + (misses == 0 ? " you are correct"
                    : " you missed " + (misses == 1 ? " one time" : " " + misses + " times")));

            // Ask for another game
            keepPlaying = userWantsAnotherGame();

        }
    }

    private boolean userWantsAnotherGame() {
        System.out.print("Do you want to start another word? Enter y or no>");
        char gameCase = this.input.nextLine().charAt(0);
        return (gameCase == 'y');
    }

    private void guess() {

        System.out.print("(Guess) Enter a letter in word " + String.valueOf(currentWord) + " ");
        char ch = this.input.nextLine().charAt(0);

        //Check if already made this guess
        for (int j = 0; j < this.currentWord.length; j++) {
            if (this.currentWord[j] == ch) {
                System.out.println(ch + " Already in the word");
                misses++;
                return;
            }
        }

        boolean isCorrect = false;
        for (int j = 0; j < this.originalWord.length; j++) {
            if (this.originalWord[j] == ch) {
                currentWord[j]=  ch;
                isCorrect = true;
            }
        }
        if (!isCorrect) {
            System.out.println(ch + " is not in the word");
            misses++;
        }
    }

    private static char[] hideRealWord(char[] originalWord) {
        char[] hiddenWord = new char[originalWord.length];
        for (int i = 0; i < originalWord.length; i++) {
            hiddenWord[i] = '*';
        }
        return hiddenWord;
    }

    private static char[] chooseWord() {
        return WORDS[(int) (Math.random() * WORDS.length)].toCharArray();
    }
}


jQuery resizable plugin Is there any way I can simplify this re-sizable solution? It seems too difficult and messy to me.

Working example

$.ui.plugin.add("resizable", "alsoResizeReverse", {

    start: function (event, ui) {

        var self = $(this).data("resizable"),
            o = self.options;

        var _store = function (exp) {
            $(exp).each(function () {
                $(this).data("resizable-alsoresize-reverse", {
                    width: parseInt($(this).width(), 10),
                    height: parseInt($(this).height(), 10),
                    left: parseInt($(this).css('left'), 10),
                    top: parseInt($(this).css('top'), 10)
                });
            });
        };

        if (typeof (o.alsoResizeReverse) == 'object' && !o.alsoResizeReverse.parentNode) {
            if (o.alsoResizeReverse.length) {
                o.alsoResize = o.alsoResizeReverse[0];
                _store(o.alsoResizeReverse);
            } else {
                $.each(o.alsoResizeReverse, function (exp, c) {
                    _store(exp);
                });
            }
        } else {
            _store(o.alsoResizeReverse);
        }
    },

    resize: function (event, ui) {
        var self = $(this).data("resizable"),
            o = self.options,
            os = self.originalSize,
            op = self.originalPosition;

        var delta = {
            height: (self.size.height - os.height) || 0,
            width: (self.size.width - os.width) || 0,
            top: (self.position.top - op.top) || 0,
            left: (self.position.left - op.left) || 0
        },

        _alsoResizeReverse = function (exp, c) {
            $(exp).each(function () {
                var el = $(this),
                    start = $(this).data("resizable-alsoresize-reverse"),
                    style = {}, css = c && c.length ? c : ['width', 'height', 'top', 'left'];

                $.each(css || ['width', 'height', 'top', 'left'], function (i, prop) {
                    var sum = (start[prop] || 0) - (delta[prop] || 0);
                    if (sum && sum >= 0) style[prop] = sum || null;
                });

                //Opera fixing relative position
                if (/relative/.test(el.css('position')) && $.browser.opera) {
                    self._revertToRelativePosition = true;
                    el.css({
                        position: 'absolute',
                        top: 'auto',
                        left: 'auto'
                    });
                }

                el.css(style);
            });
        };

        if (typeof (o.alsoResizeReverse) == 'object' && !o.alsoResizeReverse.nodeType) {
            $.each(o.alsoResizeReverse, function (exp, c) {
                _alsoResizeReverse(exp, c);
            });
        } else {
            _alsoResizeReverse(o.alsoResizeReverse);
        }
    },

    stop: function (event, ui) {
        var self = $(this).data("resizable");

        //Opera fixing relative position
        if (self._revertToRelativePosition && $.browser.opera) {
            self._revertToRelativePosition = false;
            el.css({
                position: 'relative'
            });
        }

        $(this).removeData("resizable-alsoresize-reverse");
    }
});

$(function () {

    $("#resizable").resizable({
        alsoResizeReverse: ".myframe"
    });

});
#resizable, .myframe {
    border:1px solid black;
    padding:10px;
    margin-bottom:20px;
    width:50%;
    height: 150px
}

<link href="http://ajax.googleapis.com/ajax/libs/jqueryui/1.7.0/themes/base/jquery-ui.css" rel="stylesheet"/>

This is the resizable content...

This must resize in reverse direction...

Your code is good, but there's a few things you can improve on:

            $(this).data("resizable-alsoresize-reverse", {
                width: parseInt($(this).width(), 10),
                height: parseInt($(this).height(), 10),
                left: parseInt($(this).css('left'), 10),
                top: parseInt($(this).css('top'), 10)
            });

You can use an unary instead of parseInt:

            $(this).data("resizable-alsoresize-reverse", {
                width: +$(this).width(),
                height: +$(this).height(),
                left: +$(this).css('left'),
                top: +$(this).css('top')
            });

This doesn't really look like you're storing anything.

var _store = function (exp)

It looks more like you're setting up css rules and what is exp?

You should be more descriptive with your function and variable names.

I would avoid omitting the var keyword as it makes the code harder to maintain in future.

    var self = $(this).data("resizable"),
        o = self.options,
        os = self.originalSize,
        op = self.originalPosition;

Imagine you're a contractor coming in to improve this plugin and you accidentally remove the , after self's assignment. All of a sudden you've got a bunch of troublesome globals.

Sticking to adding the var keyword on every line is a practise best kept early.

Your choice of variable assignment is strange:

Here, you assign os and op,

    var self = $(this).data("resizable"),
        o = self.options,
        os = self.originalSize,
        op = self.originalPosition;

But in the following block, you don't bother assigning self.size.* variables.

    var delta = {
        height: (self.size.height - os.height) || 0,
        width: (self.size.width - os.width) || 0,
        top: (self.position.top - op.top) || 0,
        left: (self.position.left - op.left) || 0
    },

Don't assign variables you don't need to.

Making o a shorthand accessor for your options variable is unclear, o can represent many other things. Try to be a little more clear with your variable assignment, and additionally try to avoid abbreviations.

event and ui are passed in as parameters into your three parent functions of start, resize and stop, yet they're never actually used. Remove unnecessary parameters.

You have a lot of extraneous lines, I would try to avoid that, really only leaving lines when necessary.

In your data strings, you have the following resizable-alsoresize-reverse, you should be adding a space (-) between also and resize.


Calculate co-occurrences of some words in documents dataset It's a function to calculate the co-occurrences of some words in a news dataset, such as Techcrunch, Wired. The words list could be locations, products or people names.

Words list example:

["New York","Los Angeles","Chicago"]

Return result:

{"Chicago": {"New York" : 1, "Los Angeles": 2}}

The co-cocurrence between "Chicago" and "New York" is 1.

The problems:

The code below will caculate the same co-cocurrence of two words twice. And the time cost by a test dataset with 5 articles is 13.5s.

So for a dataset of 100k articles will cost about 75 hours. Is there any better solutions to improve the performance? Thanks!

  "13.5 s ± 415 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)"

def get_co_occurrences(data):
    com = defaultdict(dict)
    for temp in data:
        for i in range(len(city_list)-1):
            for j in range(i+1, len(city_list)):
                w1, w2 = city_list[i], city_list[j]
                if " " + w1 + " " in temp and " " + w2 + " " in temp:
                    print(w1,w2)
                    if com[w1].get(w2) is None:
                        com[w1][w2] = 1
                    else:
                        com[w1][w2] += 1
    return com

Edited: Python version

Python 3.6.2 | packaged by conda-forge | (default, Jul 23 2017, 22:59:30)
[GCC 4.8.2 20140120 (Red Hat 4.8.2-15)] on linux

There is no docstring. What does the function do? What argument does it take? What does it return?
city_list is a global variable. It would be better if this were passed as a parameter to the function: this would make it easier to reuse the function or write unit tests.
The algorithm has nothing to do with cities: it would work just as well for countries, or people, or any other set of search terms. It would make this generality clearer if city_list were named something like terms.
The variable names are vague and the code would be easier to understand if they were more specific. data is a collection of documents, so documents would be clearer. temp is a document from the collection, so document would be clearer. com is a data structure containing counts of co-occurrences that will be returned as the result of the function, so a name like result would be clearer.
There are two loops over i and j such that 0 <= i < j < len(city_list). These could be combined into a single loop using itertools.combinations:

for i, j in combinations(range(len(city_list)), 2):

The only purpose of these indexes is to select the two cities. It would simpler to iterate over the cities directly, avoiding the need for their indexes.

for w1, w2 in combinations(city_list, 2):

The data structure com is a mapping from first city to mappings from second cities to counts of co-occurrences. Unless you really need all those mappings it would simpler if the data structure were a mapping from pair of cities to count of co-occurrences.
When you have a data structure containing counts of items, using collections.Counter often makes the code simpler. First, create the counter object:

result = Counter()

and then increment the count:

if " " + w1 + " " in temp and " " + w2 + " " in temp:
    result[w1, w2] += 1

Searching for terms with spaces before and after will miss matches at the beginnings and ends of documents. It would be more reliable to use a regular expression match together with the \b (word boundary) code:

if all(re.search(r'\b{}\b'.format(re.escape(term)), temp) for term in (w1, w2)):

(See the documentation for re.search and re.escape.)
The implementation in the post searches for every pair of cities in every document. If there are \$k\$ cities, \$n\$ documents, and \$w\$ words per document, then the overall runtime is \$Θ(nk^2w)\$.

An alternative approach is to check each word in each document to see if it is a city, and then to iterate over all pairs of cities found in the document. This has runtime \$O(k + n(w \log w + \min(w, k)^2))\$ since there can be \$O(w)\$ cities found in each document. This is more efficient than the original code in all cases, and much faster if (as one might expect) most words in the document are not search terms.

This could be implemented like this, by joining the search terms together into a single regular expression and then using the findall method:

from collections import Counter
from itertools import combinations
import re

def co_occurrences(documents, terms):
    """Return mapping from pairs of search terms to a count of documents
    containing both terms.

    """
    # Ensure that longer terms match in preference to shorter terms
    # that happen to be initial substrings.
    terms = sorted(terms, key=len, reverse=True)
    # Regular expression matching any of the terms, as complete words.
    search = re.compile(r'\b(?:{})\b'.format('|'.join(map(re.escape, terms))))
    result = Counter()
    for document in documents:
        matches = sorted(set(search.findall(document)))
        result.update(combinations(matches, 2))
    return result


Check if a string is palindrome or two strings are the opposite of each other I made a function which checks if a word is the opposite of each other (ex: word and drow) and then realized that with minor changes it could also be used to detect if a word is palindrome. They both return bools, false means they aren't opposite/it isn't palindrome, true means they are/it is.
First function:

bool opposite(std::string& first, std::string& second) {
    size_t size = first.size();
    if (size != second.size()) {
        return false;
    }

    for (size_t left = 0, right = size - 1; first[left] == second[right]; ++left, --right) {
        if (left == size) {
            return true;
        }
    }
    return false;
}

Second function:

bool palindrome(std::string& word) {

    for (int left = 0, right = word.size() - 1; word[left] == word[right]; ++left, --right) {
        if (right < 0) {
            return true;
        }
    }
    return false;
}

In the second function I decided to use int because size_t is unsigned, I could use left == first.size() in the if statement, but that would mean calling size() twice which I think would slow me down. The other option would be storing the value returned by first.size() in a size_t (let's call it size), using it to initialize right and changing the condition to left == size, but that would mean keeping an extra size_t variable around.

I already tested them, they work, but the question is, are there improvements I can make to speed them up?

Of course there are possible improvements. First, if you can already use C++17 std::string_view, use that for greater flexibility, specifically decoupling from how the strings are stored.

Next, minimize the number of variables involved. Take special notice that string::data() and string::size() are often a bit more involved than a simple field-access due to SSO.

I also changed the argument-names to a and b because there isn't really any difference between them which would justify a specific order.

constexpr bool opposite(std::string_view a, std::string_view b) noexcept {
    auto size = a.size();
    if(size != b.size())
        return false;
    for(auto pa = end(a), pb = begin(b); size; --size)
        if(*--pa != *pb++)
            return false;
    return true;
}

Or using standard <algorithm>s and reverse-iterators:

    return std::equal(begin(a), end(a), rbegin(b), rend(b)); // C++14
    return a.size() == b.size() && std::equal(begin(a), end(a), rbegin(b));

Apply the same to palindrome(), but also take note that if you tested the first half against the last half, there's no need to thereafter test the remainders each against each other. That would just duplicate the work.  Incidentally, that eliminates the int-variable, which might have been too small.

constexpr bool palindrome(std::string_view s) noexcept {
    for(auto left = begin(s), right = end(s); left < right;)
        if(*left++ != *--right)
            return false;
    return true;
}


Binary search tree implementation in golang I have started dabbling with golang. I wrote up a binary search tree implementation. I was wondering if there is a cleaner way. Here is my implementation:

type BNode struct {
    left  *BNode
    right *BNode
    data  int64
}

func insert(n **BNode, data int64) {
    if *n == nil {
        (*n) = &BNode{data: data, left: nil, right: nil}
    } else {
        if data <= (*n).data {
            insert(&(*n).left, data)
        } else {
            insert(&(*n).right, data)
        }
    }
}

I don't particularly like &(*n).right, is there syntax better than this? Something like &(n->right)

client code looks like:

var root *BNode = nil
insert(&root, 42)
insert(&root, 2)
insert(&root, -2)
insert(&root, 97)`

Some quick remarks:

The type BNode is public, but the insert function isn't. This is quite strange.
insert(null, 0) causes a panic. That's no good.
&BNode{data: data, left: nil, right: nil} can be just &BNode{data: data} since the default value for a pointer is null.
How would someone use a tree once you've added members to it? I'm guessing you have more functions than just insert, so would be good to share these as well.

You may want to have a separate type for the tree (that just contains a root) - this is a clean way of handling the empty tree case. In addition, insert seems more suitable as a method rather than a function, bringing the final result to something like this:

type BTree struct {
    root  *node
}

type node struct {
    left  *node
    right *node
    data  int64
}

func (t *BTree) Insert(data int64) {
    if t.root == nil {
        t.root = &node{data: data}
    } else {
        t.root.insert(data)
    }
}

func (n *node) insert(data int64) {
    if data <= n.data {
        if n.left == nil {
            n.left = &node{data: data}
        } else {
            n.left.insert(data)
        }
    } else {
        if n.right == nil {
            n.right = &node{data: data}
        } else {
            n.right.insert(data)
        }
    }
}

Usage:

var tree BTree
tree.insert(50)


C++ Caesar cipher - follow-up I asked another question the other day and decided to rewrite the code with some of the suggestions given and add functionality (instead of reading from a console I'm reading directly from a file) the code works.

Looking at it, I think the nested for loop on the encode function doesn't look that good. I tried using std::transform like the comment on the code. It worked when I used the console just to read a line, now with vectors couldn't make it work.

// Ceaser Cipher implementation

#include <iostream>
#include <string>
#include <algorithm>
#include <fstream>
#include <vector>

std::vector<std::string> encode(const std::vector<std::string> &str, int shift)
{
    std::vector<std::string> tempMsg;

    for (std::string lines : str)
    {
        // std::transform(lines.cbegin(), lines.cend(), std::back_inserter(tempMsg), [&](char ch) -> char
        for (char &ch : lines)
        {
            if (ch == 'z' || ch == 'Z')
            {
                ch -= 25;
            }
            else if (isspace(ch)) {
                ch = ' ';
            }
            else
            {
                ch += shift;
            }
        }
        tempMsg.push_back(lines);
    }

    return tempMsg;
}

std::vector<std::string> decode(const std::vector<std::string> &str, int shift)
{
    return encode(str, -1 * shift);
}

int main(int argc, char *argv[])
{
    int choice;
    std::cout << "What do you want to do? 1.Encrypt, 2.Decrypt: ";
    std::cin >> choice;

    int key;
    std::cout << "Enter desired shift: ";
    std::cin >> key;

    std::ifstream inFile("testfile.txt");
    if (!(inFile.is_open()))
    {
        std::cout << "There was a problem with the file!";
        std::exit(EXIT_FAILURE); // ExitProcess() if windows especific.
    }

    std::vector<std::string> finalResult;

    std::string line;
    std::vector<std::string> lines;

    while (std::getline(inFile, line))
    {
        lines.push_back(line);
    }
    inFile.close();

    if (choice == 1)
    {
        auto result = encode(lines, key);
        finalResult = result;
    }

    else if (choice == 2)
    {
        auto result = decode(lines, key);
        finalResult = result;
    }
    else
    {
        std::cout << "Wrong option, exiting!";
        std::exit(EXIT_FAILURE);
    }

    std::ofstream outFile("testfile.txt");
    for (auto i = finalResult.begin(); i != finalResult.end(); ++i)
    {
        outFile << *i << '\n';
    }
    outFile.close();

    std::exit(EXIT_SUCCESS);
}

In the following, I am omitting checking for the correctness of the outputs as you have not provided a specification for what that should be. Nevertheless, consider the following points.

I think the signatures of your encoding and decoding functions are a bit unexpected: I would expect an encoding function to take a single string and not a vector of strings. This gives me the feeling that the function is doing too much (i.e., violating the guideline of one function, one responsibility). So I would break the logic into smaller pieces:

std::string encode(const std::string& str, int shift)
{
    std::string str_enc;
    std::transform(str.cbegin(), str.cend(), std::back_inserter(str_enc), [&](char ch) -> char
    {
        if (ch == 'z' || ch == 'Z')
        {
            return ch - 25;
        }
        else if (isspace(ch))
        {
            return ' ';
        }
        else
        {
            return ch + shift;
        }
    });

    return str_enc;
}

std::vector<std::string> encode(const std::vector<std::string>& str, int shift)
{
    std::vector<std::string> tempMsg;
    std::transform(str.cbegin(), str.cend(), std::back_inserter(tempMsg),
        [&](const std::string& s) { return encode(s, shift); });

    return tempMsg;
}

In the main loop logic, before getting into any file reading, check whether the choice made by the user is sensible (either 1 or 2). This allows you to make finalResult const and to initialize it in a nicer way. This is a good modern practice as given by e.g., ES.28 of the C++ Core Guidelines:

assert(choice == 1 || choice == 2);
const std::vector<std::string> finalResult = [&]()
{
    return choice == 1 ? encode(lines, key) : decode(lines, key);
}();

You don't have to explicitly close file streams. They close upon destruction.
To check if the file is good to go, you can do if(inFile) { ... } invoking its operator bool() which tells you if the stream has no errors and is ready for I/O operations (specifically, it returns !fail()).
When writing to file, you can use standard algorithms (like std::copy) since your logic is very simple:

std::ofstream outFile("outfile.txt");
std::copy(finalResult.cbegin(), finalResult.cend(),
    std::ostream_iterator<std::string>(outFile, "\n"));

Everywhere in your code, avoid using std::exit (for reasons and more discussion, see here). Instead, just do return EXIT_FAILURE or return EXIT_SUCCESS as appropriate.


IMDB website query to find actors by time period I'm using data from a database from the IMDB website. The database consists of five relevant tables.

Actor (id, fname, lname, gender)
Movie (id, name, year, rank)
Director (id, fname, lname)
Cast (pid, mid, role)
Movie_Director(did, mid)

It's worth noting that the id column in Actor, Movie & Director tables is a key for the respective table.

Cast.pid refers to Actor.id, and Cast.mid refers to Movie.id.

Here's the prompt and my initial attempt at solving it. Any help in improving/speeding up the query would be greatly appreciated.

/* List all the actors who acted in at least
   one film in 2nd half of the 19th century and
   in at least one film in the 1st half of the 20th century */

    SELECT DISTINCT a.fname, a.lname
    FROM Actor a, Movie m1, Movie m2, Cast c1, Cast c2
    WHERE c1.pid = a.id
    AND c1.mid = m1.id
    AND m1.year BETWEEN 1850 AND 1900
    AND c2.pid = a.id
    AND c2.mid = m2.id
    AND m2.year BETWEEN 1901 AND 1950;

I see some of the same problems as in your last query
Do not use single-letter aliases
a, m1, c1... How about an alias that helps you write the query, instead of one that saves a few characters? That's what they are for, after all.
Old style JOIN
I think you would benefit from reading about explicit JOIN syntax instead of using the pre-ANSI-92 syntax.
Vertical white space
I personally find queries much easier to read if you use line breaks between lists of columns/values/conditions, instead of writing them inline.
Column aliases
It's good practice to rename short/ambiguous/ugly column names to something more human-friendly while presenting the result set. To you it may not matter much, but if you were presenting this report to your boss, they may scratch their head at fname and lname. The syntax for column aliases is the same as table aliases.
That Devil BETWEEN
BETWEEN is ambiguous. You should instead use logical operators.
This is about Microsoft T-SQL, but some problems apply pretty much across the board in SQL.

What do BETWEEN and the devil have in common?
I'll make no bones about it: BETWEEN is evil. For one, the meaning of the word in English does not always match the meaning of the operator in T-SQL. In T-SQL, BETWEEN is an inclusive range - not everyone gets that. Sure, in casual conversation when someone says "between 3 and 6" the answer really could be 3, 4, 5 or 6; but other times, they really mean to restrict the set to only 4 or 5 (an exclusive range).

The reviewed script
SELECT DISTINCT
    Actor.fname AS ActorFirstName,
    Actor.lname AS ActorLastName,
FROM
    Actor -- look no alias needed
    INNER JOIN Cast AS OlderCast ON OlderCast.pid = Actor.id
    INNER JOIN Movie AS OlderMovie ON OlderCast.mid = OlderMovie.id
    INNER JOIN Cast AS NewerCast ON NewerCast.pid = Actor.id
    INNER JOIN Movie AS NewerMovie ON NewerCast.mid = NewerMovie.id
WHERE
    AND OlderMovie.year >= 1850
    AND OlderMovie.year <= 1900
    AND NewerMovie.year >= 1901
    AND NewerMovie.year <= 1950;


Check if a given grid is a valid Sudoku At CodeFights I found a question about the validity of Sudoku grid. Given a grid, return true if it is valid, return false otherwise. The grid is valid when each row, each column and each 3x3 sub grid contains at most one occurrence of the numbers 1 to 9.

I solved it using C# and I would like some feedback on my solution.

The provided grid is guaranteed to be 9x9 and to only contain the characters 1 through 9 and . (for empty cells). So I did not include any error checking.

Solution

using System;

public static class Program
{
    public static void Main()
    {
        char[][] grid = {
            new char[] {'.', '.', '.', '1', '4', '.', '.', '2', '.'},
            new char[] {'.', '.', '6', '.', '.', '.', '.', '.', '.'},
            new char[] {'.', '.', '.', '.', '.', '.', '.', '.', '.'},
            new char[] {'.', '.', '1', '.', '.', '.', '.', '.', '.'},
            new char[] {'.', '6', '7', '.', '.', '.', '.', '.', '9'},
            new char[] {'.', '.', '.', '.', '.', '.', '8', '1', '.'},
            new char[] {'.', '3', '.', '.', '.', '.', '.', '.', '6'},
            new char[] {'.', '.', '.', '.', '.', '7', '.', '.', '.'},
            new char[] {'.', '.', '.', '5', '.', '.', '.', '7', '.'}
        };

        var sudoku = new Sudoku(grid);
        Console.WriteLine(sudoku.IsValid());
    }
}

public class Sudoku
{
    char[][] _grid;

    public Sudoku(char[][] grid)
    {
        _grid = grid;
    }

    public bool IsValid()
    {
        return RowsAreValid()
            && ColumnsAreValid()
            && SquaresAreValid();
    }

    bool RowsAreValid()
    {
        return Validate(GetNumberFromRow);
    }

    bool ColumnsAreValid()
    {
        return Validate(GetNumberFromColumn);
    }

    bool SquaresAreValid()
    {
        return Validate(GetNumberFromSquare);
    }

    bool Validate(Func<int, int, int> numberGetter)
    {
        for (var row = 0; row < 9; row++)
        {
            var usedNumbers = new bool[10];
            for (var column = 0; column < 9; column++)
            {
                var number = numberGetter(row, column);
                if (number != 0 && usedNumbers[number] == true)
                {
                    return false;
                }

                usedNumbers[number] = true;
            }
        }

        return true;
    }

    int GetNumberFromRow(int row, int column)
    {
        return ToNumber(_grid[row][column]);
    }

    int GetNumberFromColumn(int row, int column)
    {
        return ToNumber(_grid[column][row]);
    }

    int GetNumberFromSquare(int block, int index)
    {
        var column = 3 * (block % 3) + index % 3;
        var row = index / 3 + 3 * (block / 3);
        return ToNumber(_grid[row][column]);
    }

    int ToNumber(char c)
    {
        if (c == '.')
            return 0;
        return (int)(c - '0');
    }
}

Nice use of Func

I don't like the use of row and column in Validate and that is not really what it is

Looks like you call GetNumberFromSquare more than you need to.

Consider a byte[,] ba = new byte[9, 9]; array and use 0 for blank.

A little more compact

public class Sudoku2
{
    byte[,] gridy = new byte[9, 9] { { 0, 0, 0, 1, 4, 0, 0, 2, 0 },
                                     { 0, 0, 6, 0, 0, 0, 0, 0, 0 },
                                     { 0, 0, 0, 0, 0, 6, 0, 0, 0 },
                                     { 0, 0, 1, 0, 0, 0, 0, 0, 0 },
                                     { 0, 6, 7, 0, 0, 0, 0, 0, 9 },
                                     { 0, 0, 0, 0, 0, 0, 8, 1, 0 },
                                     { 0, 3, 0, 0, 0, 0, 0, 0, 6 },
                                     { 5, 0, 0, 0, 0, 7, 0, 0, 0 },
                                     { 0, 0, 0, 5, 0, 0, 0, 7, 0 },
                                    };
    public bool Validate(byte[,] grid)
    {
        for (int i = 0; i < 9; i++)
        {
            bool[] row = new bool[10];
            bool[] col = new bool[10];

            for (int j = 0; j < 9; j++)
            {
                if(row[grid[i,j]] & grid[i, j] > 0)
                {
                    return false;
                }
                row[grid[i, j]] = true;

                if (col[grid[j, i]] & grid[j, i] > 0)
                {
                    return false;
                }
                col[grid[j, i]] = true;

                if ((i + 3) % 3 == 0 && (j + 3) % 3 == 0)
                {
                    bool[] sqr = new bool[10];
                    for (int m = i; m < i + 3; m++)
                    {
                        for (int n = j; n < j + 3; n++)
                        {
                            if (sqr[grid[m, n]] & grid[m, n] > 0)
                            {
                                return false;
                            }
                            sqr[grid[m, n]] = true;
                        }
                    }
                }

            }
        }
        return true;
    }

    public Sudoku2()
    {
        bool good = Validate(gridy);
    }
    public Sudoku2(byte[,] grid)
    {
        bool good = Validate(grid);
    }
}


Implementing atoi() in C I was given a school assignment to implement atoi(). Please review my code.

int my_atoi(char *ato)
{
    int res;
    int sign;

    res = 0;
    sign = 1;
    while (*ato == ' ' || *ato == '\t' || *ato == '\n' || *ato == '\f' || *ato == '\r')
        ato++;
    if (*ato == '-')
        sign = -1;
    if (*ato == '-' || *ato == '+')
         ato++;
    while (*ato >= '0' && *ato <= '9')
    {
        res = res * 10 + *ato - '0';
        ato++;
    }
    return (sign * res);
}

Be consistent with stdlib.h declaration of atoi()

The standard declaration is int atoi(const char *str);. Your declaration should match (i.e., take a const char * instead of char *).

What happens if a NULL pointer is passed?

As written, you dereference ato without checking if it is NULL (0). That is a big no-no in C. At the very top of the function, you should check if it's 0 and return 0 if it is:

if (!ato) { return 0; }

Edit: Martin R points out that silently returning 0 when a NULL pointer is passed to your function, as I have suggested, is bad programming practice, because it hides the error. If there's a possibility of calling myatoi() with a NULL pointer, there's no way for the function to notify the caller of the error. Therefore, it's incumbent upon the caller to check before calling.

No need for separate variable definition and initialization

Do:

int res = 0;
int sign = 0;

Don't:

int res;
int sign;
res = 0;
sign = 1;

Don't reinvent the wheel

Your first loop that skips over whitespace could use the standard library function isspace():

Edit: spot the bug in my original code... :/

Buggy (oops): while (isspace(*ato++));
Correct:      while (isspace(*ato)) ato++;

The condition check in the second loop can use the stdlib function isdigit():

while (isdigit(*ato)) {
    ...
    ato++;
}

If your instructor or assignment instructions prevent you from using isspace() and isdigit(), why not go ahead and define them yourself? You already have the logic!*

* Except you have a minor bug in your check for space characters. You also need to check for vertical tab (\v).

Edit: Chux points out that calling isspace(*ato) and isdigit(*ato) when *ato < 0 invokes undefined behavior (UB). Specifically, the C11 standard, section 7.4, Character Handling, states,

  The header <ctype.h> declares several functions useful for classifying and mapping characters. In all cases the argument is an int, the value of which shall be representable as an unsigned char or shall equal the value of the macro EOF. If the argument has any other value, the behavior is undefined.

The correct usage, then, of the isspace() and isdigit() functions in this code would be:

while (isspace(*(unsigned char *)ato)) { atoi++; }
//...
while (isdigit(*(unsigned char *)ato)) {
    //...
    ato++;
}

Chux also pointed out that your original code as written did not have this problem; you weren't invoking undefined behavior. Moral of the story: be careful of the advice you receive from strangers on the Internet...? =)

Other than the issues mentioned, you have a decent implementation!


Probably unneeded returns This is my first "useful" Haskell program. It's invoked with filenames as arguments where the last filename is the destination to which the content of all other files will be concatenated. As far as I can tell it's working but I don't like the two returns in the main function and would like to know how I can get rid of the do in the concatFiles function.

import System.Environment (getArgs)
import System.IO (hClose, hPutStr, openFile, IOMode(WriteMode))

concatFiles :: FilePath -> [FilePath] -> IO ()
concatFiles destination files = do
    writeHandle <- openFile destination WriteMode
    mapM (writeToDestination writeHandle) files
    hClose writeHandle
    where
        writeToDestination writeHandle file =
            hPutStr writeHandle =<< readFile file

main = do
    args <- getArgs
    argc <- return $ length args
    destination <- return $ last args
    concatFiles destination $ take (argc-1) args

For your argc <- return $ length args , the reason the compiler is telling you to use the return is because in do notation, monadic binding lines must be the form:

binding <- value wrapped in whatever monad you're using

So length args just returns an Int, but Haskell wants an IO Int. By doing return $ length args, you lift the value returned by length args into the IO monad.

You are correct in smelling something funny here. Haskell provides a much cleaner way of assigning non-monadic values to be used inside of do notation with the let keyword.

You can replace those two lines with:

let argc = length args
    destination = last args

Note that bindings must have the same indentation level within a let assignment block.

But really, there is more to improve! In Haskell, the four most common list partitions are:

First element (head)
Last element (last)
Everything but the first (tail)
Everything but the last (init)

So with init we can eliminate the need for argc, changing your main to

main = do
    args <- getArgs
    let destination = last args
    concatFiles destination $ init args

But really, the word destination is longer than the function call last args, and doesn't really add much clarity, so we can change that, too, removing the need for the let entirely!

main = do
    args <- getArgs
    concatFiles (last args) (init args)

And if you really wanted to become more intimately familiar with how do notations and monads in general work, we can actually remove the do notation and replace it with a >>= operator by doing

main = getArgs >>= \args -> concatFiles (last args) (init args)

So as a quick recap of what is happening here, we are calling getArgs, which returns a list of arguments wrapped in the IO monad. The >>= (bind) operator takes that IO value, extracts the value inside, and sends it into the function on the right side of the >>= operator, where it gets bound to the argument args.


Java homework exercises working with 2D arrays For an assignment to study up on an incoming exam, I have to make these little functions. These are review style functions that make sure we know the algorithms and code necessary to do well on the test. I'm looking for anything I can improve upon in my code, so I can get the best grade possible.

  Write a method called evens that will receive a 2-D array of integers and return the sum of all the even integers in the array.

public int evens(int[][] arr) {
    int sum = 0;
    for(int[] row : arr) {
        for(int item : row) {
            if(item % 2 == 0) {
                sum += item;
            }
        }
    }
    return sum;
}

  Write a method called check that will receive a 2-D array of String and a single letter (character). The method will return true if any of the strings start with the letter that is passed in (ignoring cases)

public boolean check(String[][] arr, char c) {
    for(String[] row : arr) {
        for(String item : row) {
            if(item.charAt(0) == c) {
                return true;
            }
        }
    }
    return false;
}

  Write a method called getColumn which receives a 2-D array of doubles, and an integer parameter n. The method returns a one-dimensional array which contains the elements in the nth column of the 2-D array.

public double[] getColumn(double[][] arr, int col) { // row by column
    double[] newarr = new double[arr.length];
    for(int i = 0; i < arr.length; i++) {
        newarr[i] = arr[i][col];
    }
    return newarr;
}

  Write a method called populate that takes two parameters, m and n. The method creates and returns an m x n matrix filled with the counting numbers in row-major order starting with one.

public int[][] populate(int m, int n) {
    int[][] arr = new int[m][n];
    int count = 1;
    for(int i = 0; i < arr.length; i++) {
        for(int j = 0; j < arr[i].length; j++) {
            arr[i][j] = count;
            count++;
        }
    }
    return arr;
}

My tester class (test.java)

import java.util.Arrays;

public class test {

    public static void main(String[] args) {

        test t = new test();
        int[][] array = {
            {1, 2, 3, 4, 5},
            {6, 7, 8, 9, 10}
        };
        String[][] array2 = {
            {"abc", "def", "ghi"},
            {"jkl", "lmn", "opq"},
            {"rst", "uvw", "xyz"}
        };
        double[][] array3 = {
            {1.1, 1.2, 1.3, 1.4, 1.5},
            {1.6, 1.7, 1.8, 1.9, 2.0},
            {2.1, 2.2, 2.3, 2.4, 2.5},
            {2.6, 2.7, 2.8, 2.9, 3.0}
        };
        System.out.println("30 => " + t.evens(array));
        System.out.println();
        System.out.println("true => " + t.check(array2, 'd'));
        System.out.println();
        System.out.println("[1.1, 1.6, 2.1, 2.6] => " + Arrays.toString(t.getColumn(array3, 0)));
        System.out.println();
        System.out.println(Arrays.deepToString(t.populate(2, 5)));
    }
}

Your code looks well done.  Just a couple of tweaks to make it better:

Your check() method could fail if passed an empty string, "", since .charAt(0) will raise an IndexOutOfBoundsException.  Using the following to protect against the exception:

if ( ! item.isEmpty()  &&  item.charAt(0) == c ) {

In populate(), you loop up to arr.length and arr[i].length.  But you have just created that array of known dimensions m and n, so it would be clearer, simpler and slightly faster to use those variables for the loop limits.
Your variable names could be better: perhaps array instead of arr, and column instead of newarr.


Async.sequence implementation I have a simple implementation for "Async.sequence" (the name came from here), but I would like someone to review it for performance, tail call optimization, and functional style.

It has the following signature, which is similar to Async.Parallel:

Async<'a> list -> Async<'a list>

Async.Parallel would be ideal for performance, but it may be important to run the operations in order even though they happen to be async.

let sequence list =
    let rec loop l acc =
        async {
            match l with
            | [] ->
                return List.rev acc
            | h::t ->
                let! h' = h
                return! loop t (h'::acc)
        }
    loop list []

Update

I came up with a minimal implementation built from other basic operations: map + apply (to get map2) and retn. cons is just a helper for the :: operation, since that is not actually a function in F#.

let sequenceList list =
    List.foldBack (map2 cons) list (retn [])

It is probably better to use a library already built, but I like defining these functions myself to better understand how the operations relate. Now that I have the formulation of sequence, I can literally copy and paste this code into my Result module which also has its own map2 and retn defined, and it will work the same.

As far as I can tell, this should be properly tail-call optimized (see this post), but I haven't actually checked.

Consider using AsyncSeq, it could make dealing with asynchronous collections easier.


Is my code a 'safe' singleton? I was wondering if my code will produce a true singleton.  I am creating an Android app, and all activities should access my API through one instance of the SyncApi class.

public class Api {

    private static SyncApi api = null;

    static {
        synchronized (api) {
            if (api == null) {
                api = new ApiFActory().getSyncApi();
            }
        }
    }

    public static SyncApi getInstance() {
        return api;
    }
}

Use of the class would look like:

SyncApi api = Api.getInstance();
api.logOn("jjnguy", "pa55w0rd");

In Java there’s an established idiom for creating a thread-safe singleton, due to Bill Pugh:

public class Api {
    private Api() { }

    private static class SingletonHolder {
        public static final SyncApi INSTANCE = new ApiFactory().getSyncApi();
    }

    public static SyncApi getInstance() {
        return SingletonHolder.INSTANCE;
    }
}

This implementation is both lazy and completely thread-safe, without the need to explicit synchronization and checking. This has the advantage that it eliminates the possibility of subtle race conditions and redundant locking.


Arbitary Precision Addition I made a bigint class that can add unsigned unlimited precision integers by splitting number into chunks less than 9999, and if a part is bigger than 9999 I take mod 10000 and carry 1 to the next part.

The adding function looks a bit messy as I had to make adjustments for the algorithm if rhs has more/less digits than lhs. Is there a way to make this programm more compact and readable?

class bigint
{
    std::vector<int> parts;
public:
    bigint(unsigned long long n);
    friend std::ostream& operator<<(std::ostream& os, const bigint& n);
    bigint& operator+=(bigint n);
};

bigint::bigint(unsigned long long n)
{
    while(n != 0)
    {
        parts.push_back(n%10000);
        n/=10000;
    }
}

std::ostream& operator<<(std::ostream& os, const bigint& n)
{
    for(auto it = n.parts.rbegin(); it != n.parts.rend(); ++it)os << (*it);
    return os;
}

bigint& bigint::operator+=(bigint n)
{
    auto a = parts.begin(), b = n.parts.begin();
    int carry = 0;
    while(a != parts.end() && b != n.parts.end())
    {
        *a+=*b+carry;
        carry = 0;
        if(*a > 9999)
        {
            *a = *a%10000;
            carry = 1;
        }
        ++a; ++b;
    }

    // If rhs has more digits than lhs
    if(a == parts.end() && b != n.parts.end())
    {
        while(b != n.parts.end())
        {
            parts.push_back(0);
            a = --parts.end();
            *a+=*b+carry;
            carry = 0;
            if(*a > 9999)
            {
                *a = *a%10000;
                carry = 1;
            }
            ++a; ++b;
        }
    }

    // If lhs has more digits than rhs
    if(b == n.parts.end() && a != parts.end())
    {
        while(a != n.parts.end() && carry == 1)
        {
            *a+=carry;
            carry = 0;
            if(*a > 9999)
            {
                *a = *a%10000;
                carry = 1;
            }
            ++a;
        }
    }
    if(carry == 1)parts.push_back(1);
    return *this;
}

I have reviewed your code and here is what I've found.

Use const references where practical

The code currently declares its addition function like so:

bigint& bigint::operator+=(bigint n)

This has two problems.  First it passes by value, so a new bigint is created on every call.  This is quite wasteful of both time and memory.  Second, it should actually be a const reference.

bigint& bigint::operator+=(const bigint &n)

Eliminate "magic values"

The values of 10000 and 9999 are sprinkled through the code, but they really ought to be a named constant instead, and specifically a named constant static member.

constexpr static int MAXVAL = 10000;

(I'm using the C++11 constexpr here, but it could be const if you're using some old compiler.)

Also use the same constant everywhere.  That is, instead of writing this:

if (*a > 9999)

write this:

if (*a >= MAXVAL)

The advantage is that the compiler can probably generate more efficient code because it only has a single constant instead of two.

Be wary of signed versus unsigned

The individual parts should probably be unsigned rather than signed.  Since the sole defined constructor only takes unsigned numbers, it's reasonable to assume that the constituent parts are also unsigned.

Avoid computationally costly operations if practical

If we have two "digits" (digits in quotation marks because they're really base 10000 rather than base 10), each with a maximum value of 9999, then the maximum sum is 19998.  This means that the carry can only be 1 or 0 and that the code can be rewritten to eliminate the computationally costly % operator:

{
    *a += *b + carry;
    if (*a >= MAXVAL)
    {
        *a -= MAXVAL;
        carry = 1;
    } else {
        carry = 0;
    }
    ++a; ++b;
}

Eliminate redundant condition checks

In the addition code, the first loop only ends if one of the iterators is at the end.  This means that the next couple of lines of code:

// If rhs has more digits than lhs
if(a == parts.end() && b != n.parts.cend())
{
    while(b != n.parts.cend())
    {

Can be simplified to just the enclosed while statement.

Extract identical code into a single place

The code for the addition of digits is repeated three places, which is a very strong hit that it should probably be made into its own member function.  One way to do it:

int bigint::digitAdd(int &a, int b, int carry)
{
    a+=b+carry;
    if(a >= MAXVAL)
    {
        a -= MAXVAL;
        carry = 1;
    } else {
        carry = 0;
    }
    return carry;
}

Use for rather than while where appropriate

With the function listed above, the first loop can be rewritten as:

for(carry = 0; a != parts.end() && b != n.parts.cend(); ++a, ++b)
{
    carry = digitAdd(*a, *b, carry);
}

It's already much simpler.

Make sure your constructor always works

If the value passed is 0 the constructor will create no members in the parts vector.  You might want to reconsider your logic for that case.

Make sure your operator<< does what you really want

Consider the case in which the value stored is 10007.  The current operator<< will print "17" which is probably not what is desired.


Multiple connection string in Web API 2 I have a Web API 2 using Dapper.net, and it's accessed by various client with their own database. Currently, I'm using custom header called Client-Id to identify the client.

This Client-Id contains a ConnectionString name and I passed this connection string into my constructor.

Here's my project structure:

Api.Persentation -> Api.Service -> Api.Logic

The connection string will pass from Api.Persentation to Api.Logic as follows:

Api.Persentation

public string testApi()
{
//using the service
//consider the connection string was handled on base class
var _myService = new testApiService(base.ConnectionString);

return _myService.HelloWorld();
}

Api.Service

public class testApiService: BaseService
{
    #region const
    //This class was from API.Logic
    private readonly testApiRepo _testApiRepo;

    //the connectionString passed into base class to use a log repo
    public testApiService(string connectionString)
        : base(connectionString)
    {
        this._testApiRepo = new testApiRepo(connectionString);
    }
    #endregion

    public string HelloWorld()
    {
        //access API.Logic
        return this._testApiRepo.GetHelloWorld();
    }
}

Api.Logic

It has two classes, where BaseConnection is an abstract class to open the connection (the class how I use ConnectionString). The second one is, testApiRepo showing how I use the connection to get a database value.

/*-------------
BaseConnection Class
-------------*/
    public abstract class BaseConnection
    {
        private string _connectionString;
        public BaseConnection(string ConnectionString)
        {
            this._connectionString = ConnectionString;
        }

        public MyDbConnection GetOpenConnection(bool mars = false)
        {
            string cs = this._connectionString;
            if (mars)
            {
                SqlConnectionStringBuilder scsb = new SqlConnectionStringBuilder(cs);
                scsb.MultipleActiveResultSets = true;
                cs = scsb.ConnectionString;
            }

            //Finally we use the connection string in here
            var connection = new SqlConnection(ConfigurationManager.ConnectionStrings[this._connectionString].ToString());
            MyDbConnection DbConnection = new MyDbConnection(connection);
            DbConnection.Open();
            return DbConnection;
        }
    }

/*-------------
testApiRepo Class
-------------*/
    public class testApiRepo : BaseConnection
    {
        #region Const
        //the connection will handled on base class
        public CommissionRepo(string connectionString)
            : base(connectionString)
        {
        }
        #endregion

        public string GetHelloWorld()
        {
        //using base class to open connection
        using (var _db = base.GetOpenConnection())
        {
            string query = "SELECT 'HelloWorld'";
            var result = _db.Query<string>(query);
            return result.FirstOrDefault();
        }
    }
}

First of all I would highlight a security problem: you're directly exposing your implementation to service users. They just need to try different Client-IDs to connect to different databases. Connection string should be deduced by your application logic from client's authentication (whatever you use for this).

testApi() does not follow common .NET naming guidelines, method names should be Pascal case: TestApi() (also I would consider to use a little bit more descriptive name, TestServiceConnection(), maybe). The same apply for testApiService (TestApiService) and your local variable _myService (prefixes such as _ are controversial but - when used - they denote member fields, not local variables or parameters).

Also classes testApiService and testApiRepo has same naming issue.

BaseConnection is abstract, you'd better also declare its constructor as protected.
_connectionString is initialized only in constructor then it should be marked as readonly.
ConnectionString constructor parameter should be camel case: connectionString.

In GetOpenConnection() you're using a boolean flag mars, they should be used sparely because reading GetOpenConnection(true) you don't immediately understand what it means (true...what?) and whenever possible they should be replaced with enum or different methods. I'd also consider to do not have a default value: do you really call it so often you want to save those few characters?

Be careful when you create and return disposable objects, if something goes wrong creating MyDbConnection (again it should be myDbConnection, also better name is required) object then an exception is thrown and connection isn't immediately destroyed: when a SQL connection is popped from pool is an implementation detail you should not rely on.

All your classes are public and none of them are sealed. Will they be used outside assembly where you declared them? If not then they should not be public. Also TestApiRepo should be renamed and marked as sealed.

There isn't much more I can say about these snippets but I think you should change little bit your architecture. For example what you call BaseConnection in reality is not a connection! It's a factory class where you build connection objects (MyDbConnection) and testApiRepo has nothing to do with that factory (it should not be a derived class) but, instead, accept factory as parameter (or a connection string and then instantiate your ConnectionFactory class).


Simple random string generator in C It is just a simple piece of C code that generates random lenght random HEX srings. Any ideas how this code can be improved?

#include <stdio.h>
#include <stdlib.h>
#include <time.h>

#define MAX_SIZE 256

int main() {
    time_t t;
    srand((unsigned) time(&t));

    const char* digits = "0123456789ABCDEF";

    unsigned char str_len = rand() % MAX_SIZE;

    char* str = malloc((str_len + 1) * sizeof(char));
    if (str == NULL) {
        fprintf(stderr, "malloc failed\n");
        return -1;
    }

    char* init_ptr = str;
    char tmp_char;

    for (int i = 0; i < str_len; i++) {
        if (i == 0) {
            do {
                tmp_char = *(digits + (rand() % 16));
            } while (tmp_char == '0');
        }
        *str = tmp_char;
        tmp_char = *(digits + (rand() % 16));
        str++;
    }

    *str = '\0';

    printf("%s", init_ptr);
    free(init_ptr);

    return 0;
}

You are effectively picking your first hex digit from "123456789ABCDEF" (no zero) and the subsequent digits from "0123456789ABCDEF".  Hence it might be better to adjust the first random pick:

// This assumes that str_len is at least 1.
*str = *(digits + (1 + rand() % 15));  // Pick first char in range 1..15
str++;

// For loop starts at 1 as str[0] already picked.
for (int i = 1; i < str_len; i++) {
    *str = *(digits + (rand() % 16));  // Following chars in range 0..15
    str++;
}

That eliminates the tmp_char variable and the while loop, which makes the code a lot cleaner, to my eye anyway.


Bash script to set up an ad hoc wireless network This script creates an adhoc network using hostapd.
I have tested it and seems to work reliably.
I am new to linux networking and not sure if this is a recommended way to create an adhoc network this way.

#!/bin/bash
#set -x
DEVICE=wlan0
CONFIG_FILE=./hostapd.conf
ConfigureDevice()
{
    if ! sudo iwconfig 2>/dev/null | grep $DEVICE >/dev/null;then
        echo $DEVICE not found!
        exit -1
    fi

    #Ensure config file exists
    if [ ! -e $CONFIG_FILE ]
    then
        echo "Can't find hostapd config file"
        exit -1
    fi

    sudo service isc-dhcp-server stop
    sudo pkill hostapd
    sudo ifdown  $DEVICE
    sudo ifconfig $DEVICE up 192.168.1.100 netmask 255.255.255.0
    sudo service isc-dhcp-server start
    sudo hostapd -B $CONFIG_FILE

}
cd ${0%/*}  #make current working directory the directory of the bash script
ConfigureDevice

Not answering your actual question, but offering a couple of code comments:

use cd -- "$(dirname -- "$0")"

if the script is in the PATH and someone just enters the script name, you will try to cd to the (probably non-existant) "script name" directory in the current dir.

use grep -q $DEVICE

that is a bit speedier since, if it finds a match, it exits immediately instead of having to read the entire input looking for all matches.


Stack implementation with shared_ptr C++11 stack.hpp

#ifndef _TOP_HPP
#define _TOP_HPP

#include<memory>
#include<iostream>
#include<cstring>

    template <typename T>
    class Stack{
        private:
            struct Node{
                std::shared_ptr<Node> prev;
                T value;
            };

            int size;
            std::shared_ptr<Node> top;
            const char* name;

        public:
            Stack();
            Stack(const char* name);
            bool isEmpty()const;
            void push(T);
            const int pop();
            const int getSize()const{return size;}
            const char& getName()const{return name;}
            void clear();

            Stack<T>& operator=(const Stack<T>&);
            //don't know why but when implement this friend fun beyond
            //class then main.cpp don't compile
            friend std::ostream&  operator<<(std::ostream& os,const Stack<T>& s){
                if(strcmp(s.name,"null")) os<<"Name: "<<s.name<<"  Size "<<s.size<<std::endl;
                auto temp=s.top.get();
                while(temp){
                    os<<temp->value<<std::endl;
                    temp=temp->prev.get();
                }
                return os<<std::endl;
            }

    };

    template <typename T>
    inline Stack<T>::Stack():size(0),top(std::make_shared<Node>()),name("null"){
        top->prev=nullptr;
        top->value=0;
    }

    template <typename T>
    inline Stack<T>::Stack(const char* name):size(0),top(std::make_shared<Node>()),name(name){
        top->prev=nullptr;
        top->value=0;
    }

    template <typename T>
    inline bool Stack<T>::isEmpty()const{
        if(size==0) return true;
        return false;
    }

    template <typename T>
    void Stack<T>::push(T x){
        std::shared_ptr<Node> add =std::make_shared<Node>();
        add->value=x;
        add->prev=nullptr;
        if(isEmpty()) top=std::move(add);
        else{
            add->prev=std::move(top);
            top=std::move(add);
        }
        ++size;
    }

    template <typename T>
    const int Stack<T>::pop(){
        int p=top->value;
        if(isEmpty()){ std::cerr<<"stack is empty"<<std::endl; }
        else{
            top=top->prev; //this is good idea ?
            --size;
        }
            return p;
    }

     template <typename T>
     Stack<T>& Stack<T>::operator=(const Stack<T>& s){
         if(this!=&s){
             size=s.size;
             top=s.top;
         }
         return *this;
     }

    template <typename T>
    inline void Stack<T>::clear(){
        while(top){
            top=std::move(top->prev);i//how can i improve this ?
            --size;
        }
    }

#endif

main.cpp

#include<iostream>
#include<memory>
#include "stack.hpp"

using namespace std;

int main(){

    Stack<int> s("asd");
    Stack<int> q;
    s.push(12);
    s.push(13);
    s.push(14);
    s.push(15);

    q=s;

    cout<<s; //cout's normal display a stacks
    cout<<q;
    s.clear(); // after this fun on s

    cout<<s; // this stack is delete
    cout<<q; // this stack have only 15 (top of stack s) ;/

}

Use containers

Instead of char* name use std::string name, it's safer with the cost of very little overhead, and prevents basic errors like assigning a char string to a pointer, like you do in your constructors, for example:

template <typename T>
inline Stack<T>::Stack(const char* name) :size(0), top(std::make_shared<Node>()), name(name)

You assign a pointer to a pointer, without any control over the lifetime of memory for name, and you'd get undefined behavior when you try to use it.

Always use brackets

Instead of

if (isEmpty()) top = std::move(add);
else {
    add->prev = std::move(top);
    top = std::move(add);
}

You should write

if (isEmpty()) {
    top = std::move(add);
}
else {
    add->prev = std::move(top);
    top = std::move(add);
}

It makes the code more readable and would prevent mistakes that are too easy to miss. There're several places like this in the code.

Your copy constructor is bad

You're not doing a deep copy, but a shallow copy. q gets shared ownership of s stack, you should make a deep copy of it. For example, when you call clear on s, the internal stack gets destroyed, however, q is now in inconsistent state. Here's an example of your stack deep copy

template <typename T>
Stack<T>& Stack<T>::operator=(const Stack<T>& s) {
    if (this != &s) {
        //Creating a new stack
        top = std::make_shared<Node>();

        //Copying the top value
        top->value = s.top->value;
        std::shared_ptr<Node> s_stack = s.top;
        top->prev = std::make_shared<Node>();
        std::shared_ptr<Node> this_prev = top;

        //Copying next values
        while (s_stack->prev) {
            this_prev = this_prev->prev;
            s_stack = s_stack->prev;
            this_prev->value = s_stack->value;
            this_prev->prev = std::make_shared<Node>();
        }

        this_prev->prev = nullptr;
        size = s.size;
    }
    return *this;
}

Note that further improvements can be made if your Node class would be more intelligent and provide a constructor that would initialize the value, but set the prev pointer to nullptr.

Clear function is bad

After clear the expected behavior is that s is reset to the initial state, that is, top is a shared_ptr to Node object with value = 0, however, after clear method top is a shared_ptr to nullptr. This should be a better implementation that wouldn't mess with other shared owners of internal stack.

template <typename T>
inline void Stack<T>::clear() {
    top = std::make_shared<Node>();
    top->prev = nullptr;
    top->value = 0;
    size = 0;
}

However, it requires a correct implementation of copy constructor, since now q memory wouldn't be destroyed, that's what shared_ptr expresses.

PS. You could also gain more insight if you would add a destructor to Node which would output something when a corresponding Node object is destroyed, something like this, but for learning purposes only, it's not a production code in any way.

~Node() {
    std::cout << "destroyed\n";
}